Finch experiment: auto-detect text encoding

Finch experiment: auto-detect text encoding

Experiment b/518968 aims to measure the impact of turning
on auto-encoding detection on Chrome on Android by default.

- Adds methods that tell us if:
  1) auto-encoding detection was attempted, due to lacking
     encoding information from meta tag, header, BOM, etc.
  2) auto-encoding detection successfully detected a new
     encoding which is different from a default one,
     hence would show the page being browsed which would
     otherwise have shown garbled text.

- Selectively turns on text encoding auto-detection by
  default for experiment group.

- Uploads histogram data on the auto-detection logic
  triggering rate and encoding method detected by
  the logic.

The CL will be reverted once the experiment is finished.

BUG=518968
Committed: https://crrev.com/35a1421d891beb357d5e29cf3f5fb553f436e055
Cr-Commit-Position: refs/heads/master@{#363716}

[Jinsuk Kim, 2015-11-18 01:53:23]

jinsukkim@chromium.org changed reviewers:
+ aelias@chromium.org, kkimlabs@chromium.org, twellington@chromium.org

[Theresa, 2015-11-18 18:38:34]

lgtm

[Jinsuk Kim, 2015-11-18 23:12:36]

Thanks for the review!

Alex, would you help me find right people (in Blink team) who would review this
CL?

[aelias_OOO_until_Jul13, 2015-11-19 01:27:02]

aelias@chromium.org changed reviewers:
+ esprehn@chromium.org

[aelias_OOO_until_Jul13, 2015-11-19 01:28:39]

esphehn@ might be appropriate OWNER as this patch is mainly about plumbing some
info through layers of the Blink "onion".  Going through WebDocument looks a bit
odd to me, although I don't have an immediate idea for where else it might fit.

[Jinsuk Kim, 2015-11-19 01:31:35]

Description was changed from

==========
Add methods to get auto-encoding detection result

Experiment b/518968 aims to measure the impact of turning on
auto-encoding detection on Chrome on Android by default.

The methods added in this CL tell us if:
1) auto-encoding detection was attempted, due to lacking encoding
   information from meta tag, header, BOM, etc.
2) auto-encoding detection successfully detected a new encoding
   which is different from a default one, hence would show the page
   being browsed which would otherwise have shown garbled text.

BUG=518968
==========

to

==========
Add methods to get auto-encoding detection result

Experiment b/518968 aims to measure the impact of turning on
auto-encoding detection on Chrome on Android by default.

The methods added in this CL tell us if:
1) auto-encoding detection was attempted, due to lacking encoding
   information from meta tag, header, BOM, etc.
2) auto-encoding detection successfully detected a new encoding
   which is different from a default one, hence would show the page
   being browsed which would otherwise have shown garbled text.

The CL will be reverted once the experiment is finished.
BUG=518968
==========

[Jinsuk Kim, 2015-11-19 01:38:53]

On 2015/11/19 01:28:39, aelias wrote:
> esphehn@ might be appropriate OWNER as this patch is mainly about plumbing
some
> info through layers of the Blink "onion".  Going through WebDocument looks a
bit
> odd to me, although I don't have an immediate idea for where else it might
fit.

Thanks. Put the methods in WebDocument to use them in a renderer when it
finishes
loading a document.

Elliot, would you take a look? This is a temporary change for an experiment
crbug.com/518968 to get the result of the universal encoding detector.

[aelias_OOO_until_Jul13, 2015-11-24 22:10:22]

Elliot, quick ping for review, in case this fell off your list of things.  (BTW,
I'm thinking we will need more public/ OWNERS for Onion Soup, as everyone
available to review CLs like this seem overloaded.)

[aelias_OOO_until_Jul13, 2015-12-01 01:11:13]

aelias@chromium.org changed reviewers:
+ tkent@chromium.org
- esprehn@chromium.org

[aelias_OOO_until_Jul13, 2015-12-01 01:11:38]

Adding tkent@ for OWNERS since Elliot doesn't seem to have the cycles.

[tkent, 2015-12-01 01:18:02]

lgtm

https://codereview.chromium.org/1456843002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/DocumentEncodingData.h (right):

https://codereview.chromium.org/1456843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/DocumentEncodingData.h:53: private:
nit: add a blink line before |private:|.

https://codereview.chromium.org/1456843002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebDocument.cpp (right):

https://codereview.chromium.org/1456843002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebDocument.cpp:342:
WebDocument::WebDocument(const PassRefPtrWillBeRawPtr<Document>& elem)
nit: add a blank line between functions.

[Jinsuk Kim, 2015-12-01 02:13:52]

Thanks for the review!

https://codereview.chromium.org/1456843002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/DocumentEncodingData.h (right):

https://codereview.chromium.org/1456843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/DocumentEncodingData.h:53: private:
On 2015/12/01 01:18:02, tkent wrote:
> nit: add a blink line before |private:|.

Done.

https://codereview.chromium.org/1456843002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebDocument.cpp (right):

https://codereview.chromium.org/1456843002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebDocument.cpp:342:
WebDocument::WebDocument(const PassRefPtrWillBeRawPtr<Document>& elem)
On 2015/12/01 01:18:02, tkent wrote:
> nit: add a blank line between functions.

Done.

[esprehn, 2015-12-01 03:19:10]

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

[esprehn, 2015-12-01 03:19:10]

This needs tests, how long is this experiment too? What release will you remove
it?

[Jinsuk Kim, 2015-12-01 03:48:47]

On 2015/12/01 03:19:10, esprehn wrote:
> This needs tests, how long is this experiment too? What release will
> you remove it?

I think a month would be enough. My plan is to run the experiment in 49
(feature freeze Dec/18) and remove it before 50 is released.

[Jinsuk Kim, 2015-12-01 03:48:48]

On 2015/12/01 03:19:10, esprehn wrote:
> This needs tests, how long is this experiment too? What release will
> you remove it?

I think a month would be enough. My plan is to run the experiment in 49
(feature freeze Dec/18) and remove it before 50 is released.

[esprehn, 2015-12-01 18:55:34]

What do you plan to do with this data? Is it just to record an UMA somewhere? Do
you have an example usage? This patch only touches webkit/ code, but we're in
one repository now, can you add the code that uses it too?

[Jinsuk Kim, 2015-12-01 20:26:37]

On 2015/12/01 18:55:34, esprehn wrote:
> What do you plan to do with this data? Is it just to record
> an UMA somewhere? Do you have an example usage? This patch
> only touches webkit/ code, but we're in one repository now,
> can you add the code that uses it too?

The patch on the Clank side is up for review:
https://codereview.chromium.org/1456843002/
The plan is detailed in the bug. The data will be used to get the ratio of pages
for which autodetection gets triggered and the detected text encoding methods. I
suspect the ratio is not significant, hence the impact of autodetection (loading
time) won't affect majority of web pages. This experiment will let me get the
magnitude and help us make the decision (turning on the autodetection by
default).

[esprehn, 2015-12-01 20:33:21]

On 2015/12/01 at 20:26:37, jinsukkim wrote:
> On 2015/12/01 18:55:34, esprehn wrote:
> > What do you plan to do with this data? Is it just to record
> > an UMA somewhere? Do you have an example usage? This patch
> > only touches webkit/ code, but we're in one repository now,
> > can you add the code that uses it too?
> 
> The patch on the Clank side is up for review:
https://codereview.chromium.org/1456843002/
> The plan is detailed in the bug. The data will be used to get the ratio of
pages for which autodetection gets triggered and the detected text encoding
methods. I suspect the ratio is not significant, hence the impact of
autodetection (loading time) won't affect majority of web pages. This experiment
will let me get the magnitude and help us make the decision (turning on the
autodetection by default).

You just linked this patch again, what's the right link?

[Jinsuk Kim, 2015-12-01 21:07:27]

On 2015/12/01 20:33:21, esprehn wrote:
> You just linked this patch again, what's the right link?

Sorry the right link is https://codereview.chromium.org/1482173002/

[Jinsuk Kim, 2015-12-02 09:46:03]

jinsukkim@chromium.org changed reviewers:
+ battre@chromium.org, jwd@chromium.org

[Jinsuk Kim, 2015-12-02 09:48:28]

Description was changed from

==========
Add methods to get auto-encoding detection result

Experiment b/518968 aims to measure the impact of turning on
auto-encoding detection on Chrome on Android by default.

The methods added in this CL tell us if:
1) auto-encoding detection was attempted, due to lacking encoding
   information from meta tag, header, BOM, etc.
2) auto-encoding detection successfully detected a new encoding
   which is different from a default one, hence would show the page
   being browsed which would otherwise have shown garbled text.

The CL will be reverted once the experiment is finished.
BUG=518968
==========

to

==========
Finch experiment: auto-detect text encoding

Experiment b/518968 aims to measure the impact of turning 
on auto-encoding detection on Chrome on Android by default.

- Adds methods that tell us if:
  1) auto-encoding detection was attempted, due to lacking 
     encoding information from meta tag, header, BOM, etc.
  2) auto-encoding detection successfully detected a new 
     encoding which is different from a default one,
     hence would show the page being browsed which would 
     otherwise have shown garbled text.
    
- Selectively turns on text encoding auto-detection by
  default for experiment group.
    
- Uploads histogram data on the auto-detection logic
  triggering rate and encoding method detected by
  the logic.

The CL will be reverted once the experiment is finished.

BUG=518968
==========

[battre, 2015-12-02 13:08:26]

chrome/browser/ui/prefs/prefs_tab_helper.cc LGTM with comment

https://codereview.chromium.org/1456843002/diff/40001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/1456843002/diff/40001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:323: bool
IsAutodetectEncodingEnabled() {
nit: Should this be IsAutodetectEncodingEnabledByDefault()?

https://codereview.chromium.org/1456843002/diff/40001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:595: true :
l10n_util::GetStringUTF8(IDS_USES_UNIVERSAL_DETECTOR) == "true";
... = IsAutodetectEncodingEnabled() ||
l10n_util::GetStringUTF8(IDS_USES_UNIVERSAL_DETECTOR) == "true";

[Jinsuk Kim, 2015-12-02 23:14:38]

Thanks for the review!

Will add tests in a follow-up CL.

https://codereview.chromium.org/1456843002/diff/40001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/1456843002/diff/40001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:323: bool
IsAutodetectEncodingEnabled() {
On 2015/12/02 13:08:26, battre wrote:
> nit: Should this be IsAutodetectEncodingEnabledByDefault()?

Done.

https://codereview.chromium.org/1456843002/diff/40001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:595: true :
l10n_util::GetStringUTF8(IDS_USES_UNIVERSAL_DETECTOR) == "true";
On 2015/12/02 13:08:26, battre wrote:
> ... = IsAutodetectEncodingEnabled() ||
> l10n_util::GetStringUTF8(IDS_USES_UNIVERSAL_DETECTOR) == "true";

Done.

[Jinsuk Kim, 2015-12-03 22:46:21]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2015-12-03 22:46:22]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
twellington@chromium.org, battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1456843002/#ps60001
(title: "addressed comments")

[commit-bot: I haz the power, 2015-12-03 22:47:08]

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

[commit-bot: I haz the power, 2015-12-04 00:53:22]

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

[commit-bot: I haz the power, 2015-12-04 00:53:23]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

[Jinsuk Kim, 2015-12-04 04:43:15]

The CQ bit was checked by jinsukkim@chromium.org

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

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

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

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

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

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

[jwd, 2015-12-07 23:43:08]

histograms lgtm.

https://codereview.chromium.org/1456843002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1456843002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:58044: +<enum name="EncodingMethod"
type="int">
Alternatively, you could hash the encoding name, and log it to a sparse
histogram. You'd still populate this enum, with the hash to label mapping. Any
encoding methods you missed would show up as unlabeled buckets, and you could
update histogram.xml when you figure out what they are. The added cost is that
writing to a sparse histogram is more expensive (requires a lock), and all the
unknowns won't be counted in one place.

Up to you. I'm fine with how it is.

[Jinsuk Kim, 2015-12-08 00:09:42]

Thanks for the review!

https://codereview.chromium.org/1456843002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1456843002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:58044: +<enum name="EncodingMethod"
type="int">
On 2015/12/07 23:43:07, Jesse Doherty wrote:
> Alternatively, you could hash the encoding name, and log it to a sparse
> histogram. You'd still populate this enum, with the hash to label mapping. Any
> encoding methods you missed would show up as unlabeled buckets, and you could
> update histogram.xml when you figure out what they are. The added cost is that
> writing to a sparse histogram is more expensive (requires a lock), and all the
> unknowns won't be counted in one place.
> 
> Up to you. I'm fine with how it is.

Thanks for letting me know about sparse histogram which I was not aware of. I
think enum only is simple and enough for the experiment. Let me keep what's been
done here.

[Jinsuk Kim, 2015-12-08 01:03:24]

The CQ bit was checked by jinsukkim@chromium.org

[commit-bot: I haz the power, 2015-12-08 01:08:23]

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

[commit-bot: I haz the power, 2015-12-08 02:43:51]

Description was changed from

==========
Finch experiment: auto-detect text encoding

Experiment b/518968 aims to measure the impact of turning 
on auto-encoding detection on Chrome on Android by default.

- Adds methods that tell us if:
  1) auto-encoding detection was attempted, due to lacking 
     encoding information from meta tag, header, BOM, etc.
  2) auto-encoding detection successfully detected a new 
     encoding which is different from a default one,
     hence would show the page being browsed which would 
     otherwise have shown garbled text.
    
- Selectively turns on text encoding auto-detection by
  default for experiment group.
    
- Uploads histogram data on the auto-detection logic
  triggering rate and encoding method detected by
  the logic.

The CL will be reverted once the experiment is finished.

BUG=518968
==========

to

==========
Finch experiment: auto-detect text encoding

Experiment b/518968 aims to measure the impact of turning 
on auto-encoding detection on Chrome on Android by default.

- Adds methods that tell us if:
  1) auto-encoding detection was attempted, due to lacking 
     encoding information from meta tag, header, BOM, etc.
  2) auto-encoding detection successfully detected a new 
     encoding which is different from a default one,
     hence would show the page being browsed which would 
     otherwise have shown garbled text.
    
- Selectively turns on text encoding auto-detection by
  default for experiment group.
    
- Uploads histogram data on the auto-detection logic
  triggering rate and encoding method detected by
  the logic.

The CL will be reverted once the experiment is finished.

BUG=518968
==========

[commit-bot: I haz the power, 2015-12-08 02:43:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-08 02:44:49]

Description was changed from

==========
Finch experiment: auto-detect text encoding

Experiment b/518968 aims to measure the impact of turning 
on auto-encoding detection on Chrome on Android by default.

- Adds methods that tell us if:
  1) auto-encoding detection was attempted, due to lacking 
     encoding information from meta tag, header, BOM, etc.
  2) auto-encoding detection successfully detected a new 
     encoding which is different from a default one,
     hence would show the page being browsed which would 
     otherwise have shown garbled text.
    
- Selectively turns on text encoding auto-detection by
  default for experiment group.
    
- Uploads histogram data on the auto-detection logic
  triggering rate and encoding method detected by
  the logic.

The CL will be reverted once the experiment is finished.

BUG=518968
==========

to

==========
Finch experiment: auto-detect text encoding

Experiment b/518968 aims to measure the impact of turning
on auto-encoding detection on Chrome on Android by default.

- Adds methods that tell us if:
  1) auto-encoding detection was attempted, due to lacking
     encoding information from meta tag, header, BOM, etc.
  2) auto-encoding detection successfully detected a new
     encoding which is different from a default one,
     hence would show the page being browsed which would
     otherwise have shown garbled text.

- Selectively turns on text encoding auto-detection by
  default for experiment group.

- Uploads histogram data on the auto-detection logic
  triggering rate and encoding method detected by
  the logic.

The CL will be reverted once the experiment is finished.

BUG=518968

Committed: https://crrev.com/35a1421d891beb357d5e29cf3f5fb553f436e055
Cr-Commit-Position: refs/heads/master@{#363716}
==========

[commit-bot: I haz the power, 2015-12-08 02:44:51]

Patchset 4 (id:??) landed as
https://crrev.com/35a1421d891beb357d5e29cf3f5fb553f436e055
Cr-Commit-Position: refs/heads/master@{#363716}

[falken, 2015-12-08 04:11:04]

On 2015/12/08 02:44:51, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/35a1421d891beb357d5e29cf3f5fb553f436e055
> Cr-Commit-Position: refs/heads/master@{#363716}

It looks like this broke Linux MSAN:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b...

STDERR: ==11==WARNING: MemorySanitizer: use-of-uninitialized-value
STDERR:     #0 0x7fa20c129553 in operator!=
third_party/WebKit/Source/core/dom/DocumentEncodingData.h:70:9
STDERR:     #1 0x7fa20c129553 in updateDocument
third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:157:0
STDERR:     #2 0x7fa20c1296ef in appendRawBytesFromMainThread
third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:131:5