Remove document.defaultCharset

Remove document.defaultCharset

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/pWSb_tq13Kg/Dmk59Fb9AQAJ

The removed Android test depended on defaultCharset. There is already
another test that verifies that the setting affects the default
encoding: LayoutTests/http/tests/download/default-encoding.html

BUG=567738
Committed: https://crrev.com/0bb2dcd18efa71df5a93c7a5f4075cb876fa2f86
Cr-Commit-Position: refs/heads/master@{#376170}

[philipj_slow, 2016-02-16 19:19:47]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-16 19:23:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707473002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707473002/1

[commit-bot: I haz the power, 2016-02-16 22:16:19]

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

[commit-bot: I haz the power, 2016-02-16 22:16:19]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[philipj_slow, 2016-02-17 03:11:38]

Description was changed from

==========
Remove document.defaultCharset

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/pWSb_tq13Kg/Dmk59Fb9...

BUG=567738
==========

to

==========
Remove document.defaultCharset

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/pWSb_tq13Kg/Dmk59Fb9...

The removed Android test depended on defaultCharset. There is already
another test that verifies that the setting affects the default
encoding: LayoutTests/http/tests/download/default-encoding.html

BUG=567738
==========

[philipj_slow, 2016-02-17 11:32:21]

philipj@opera.com changed reviewers:
+ mnaganov@chromium.org, tkent@chromium.org

[philipj_slow, 2016-02-17 11:32:22]

mnaganov@, can you please review android_webview/? See the description for why I
think it's reasonable to remove the test, but other options welcome.

tkent@, can you please review third_party/WebKit/?

[mnaganov (inactive), 2016-02-17 16:42:48]

Please don't remove the code, just disable the test, as I advised in the
comment. I will rework the test, as we also verify that changing the property
value in one WebView doesn't affect other WebViews.

LGTM % comment

https://codereview.chromium.org/1707473002/diff/40001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(left):

https://codereview.chromium.org/1707473002/diff/40001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1607:
@Feature({"AndroidWebView", "Preferences"})
Please do not remove any code. I will rework the test to avoid using this
property of `document`.

Instead, please change these annotations as follows:

/*
 * Disabled due to document.defaultCharset removal. crbug.com/587484
 * @SmallTest
 * @Feature({"AndroidWebView", "Preferences"})
 */
@DisabledTest

and add an import if needed:

import org.chromium.base.test.util.DisabledTest;

[philipj_slow, 2016-02-17 16:52:09]

https://codereview.chromium.org/1707473002/diff/40001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
(left):

https://codereview.chromium.org/1707473002/diff/40001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1607:
@Feature({"AndroidWebView", "Preferences"})
On 2016/02/17 16:42:48, mnaganov wrote:
> Please do not remove any code. I will rework the test to avoid using this
> property of `document`.
> 
> Instead, please change these annotations as follows:
> 
> /*
>  * Disabled due to document.defaultCharset removal. crbug.com/587484
>  * @SmallTest
>  * @Feature({"AndroidWebView", "Preferences"})
>  */
> @DisabledTest
> 
> and add an import if needed:
> 
> import org.chromium.base.test.util.DisabledTest;

Thanks for the guidance, I've made that change. I haven't tried to run the test,
so will start a dry run to see if I got it right.

[philipj_slow, 2016-02-17 16:52:17]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-17 16:52:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707473002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707473002/60001

[mnaganov (inactive), 2016-02-17 16:53:56]

On 2016/02/17 16:52:09, philipj_UTC7 wrote:
>
https://codereview.chromium.org/1707473002/diff/40001/android_webview/javates...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
> (left):
> 
>
https://codereview.chromium.org/1707473002/diff/40001/android_webview/javates...
>
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1607:
> @Feature({"AndroidWebView", "Preferences"})
> On 2016/02/17 16:42:48, mnaganov wrote:
> > Please do not remove any code. I will rework the test to avoid using this
> > property of `document`.
> > 
> > Instead, please change these annotations as follows:
> > 
> > /*
> >  * Disabled due to document.defaultCharset removal. crbug.com/587484
> >  * @SmallTest
> >  * @Feature({"AndroidWebView", "Preferences"})
> >  */
> > @DisabledTest
> > 
> > and add an import if needed:
> > 
> > import org.chromium.base.test.util.DisabledTest;
> 
> Thanks for the guidance, I've made that change. I haven't tried to run the
test,
> so will start a dry run to see if I got it right.

These tests are on CQ, so you can just try committing.

[philipj_slow, 2016-02-17 17:00:15]

philipj@opera.com changed reviewers:
+ isherman@chromium.org

[philipj_slow, 2016-02-17 17:00:16]

isherman@, it doesn't show up in this CL, but the only remaining mention of
defaultCharset is now in
https://chromium.googlesource.com/chromium/src/+/7bc34f43f0c06e849031bab4abaa...

Can you advise if anything should be done about this? It's actually populated
from `std::string charset = user_prefs->GetString(::prefs::kDefaultCharset)` in
risk_util.cc, so should I just update the documentation?

[philipj_slow, 2016-02-17 17:02:00]

On 2016/02/17 16:53:56, mnaganov wrote:
> On 2016/02/17 16:52:09, philipj_UTC7 wrote:
> >
>
https://codereview.chromium.org/1707473002/diff/40001/android_webview/javates...
> > File
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java
> > (left):
> > 
> >
>
https://codereview.chromium.org/1707473002/diff/40001/android_webview/javates...
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1607:
> > @Feature({"AndroidWebView", "Preferences"})
> > On 2016/02/17 16:42:48, mnaganov wrote:
> > > Please do not remove any code. I will rework the test to avoid using this
> > > property of `document`.
> > > 
> > > Instead, please change these annotations as follows:
> > > 
> > > /*
> > >  * Disabled due to document.defaultCharset removal. crbug.com/587484
> > >  * @SmallTest
> > >  * @Feature({"AndroidWebView", "Preferences"})
> > >  */
> > > @DisabledTest
> > > 
> > > and add an import if needed:
> > > 
> > > import org.chromium.base.test.util.DisabledTest;
> > 
> > Thanks for the guidance, I've made that change. I haven't tried to run the
> test,
> > so will start a dry run to see if I got it right.
> 
> These tests are on CQ, so you can just try committing.

Right, but I'm still waiting for review on third_party/WebKit/

[commit-bot: I haz the power, 2016-02-17 19:47:49]

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

[commit-bot: I haz the power, 2016-02-17 19:47:49]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ilya Sherman, 2016-02-17 22:49:16]

On 2016/02/17 17:00:16, philipj_UTC7 wrote:
> isherman@, it doesn't show up in this CL, but the only remaining mention of
> defaultCharset is now in
>
https://chromium.googlesource.com/chromium/src/+/7bc34f43f0c06e849031bab4abaa...
> 
> Can you advise if anything should be done about this? It's actually populated
> from `std::string charset = user_prefs->GetString(::prefs::kDefaultCharset)`
in
> risk_util.cc, so should I just update the documentation?

Would it be possible to replace this with some equivalent property?  If there is
no equivalent property, then it probably just makes sense to remove it.  Is
::prefs::kDefaultCharset used for any other purpose, or just by the autofill
risk code?

[philipj_slow, 2016-02-18 01:31:14]

On 2016/02/17 22:49:16, Ilya Sherman wrote:
> On 2016/02/17 17:00:16, philipj_UTC7 wrote:
> > isherman@, it doesn't show up in this CL, but the only remaining mention of
> > defaultCharset is now in
> >
>
https://chromium.googlesource.com/chromium/src/+/7bc34f43f0c06e849031bab4abaa...
> > 
> > Can you advise if anything should be done about this? It's actually
populated
> > from `std::string charset = user_prefs->GetString(::prefs::kDefaultCharset)`
> in
> > risk_util.cc, so should I just update the documentation?
> 
> Would it be possible to replace this with some equivalent property?  If there
is
> no equivalent property, then it probably just makes sense to remove it.  Is
> ::prefs::kDefaultCharset used for any other purpose, or just by the autofill
> risk code?

kDefaultCharset is used to set Blink's defaultTextEncodingName setting (via
WebPreferences::default_encoding) and that is what defaultCharset used.
defaultTextEncodingName is also used in
TextResourceDecoderBuilder::createDecoderInstance, as the fallback encoding for
scripts and stylesheets. That still remains and is tested by
LayoutTests/http/tests/download/default-encoding.html

I've updated fingerprint.proto to simply not refer to defaultCharset, PTAL.

[Ilya Sherman, 2016-02-18 01:38:19]

lgtm

[jochen (gone - plz use gerrit), 2016-02-18 13:46:48]

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

[jochen (gone - plz use gerrit), 2016-02-18 13:46:49]

lgtm

[philipj_slow, 2016-02-18 14:21:50]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2016-02-18 14:21:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
mnaganov@chromium.org
Link to the patchset: https://codereview.chromium.org/1707473002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-02-18 14:22:45]

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

[commit-bot: I haz the power, 2016-02-18 15:35:25]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-18 15:36:22]

Description was changed from

==========
Remove document.defaultCharset

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/pWSb_tq13Kg/Dmk59Fb9...

The removed Android test depended on defaultCharset. There is already
another test that verifies that the setting affects the default
encoding: LayoutTests/http/tests/download/default-encoding.html

BUG=567738
==========

to

==========
Remove document.defaultCharset

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/pWSb_tq13Kg/Dmk59Fb9...

The removed Android test depended on defaultCharset. There is already
another test that verifies that the setting affects the default
encoding: LayoutTests/http/tests/download/default-encoding.html

BUG=567738

Committed: https://crrev.com/0bb2dcd18efa71df5a93c7a5f4075cb876fa2f86
Cr-Commit-Position: refs/heads/master@{#376170}
==========

[commit-bot: I haz the power, 2016-02-18 15:36:24]

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