Add integration of the new heuristics

Add integration of the new heuristics

This adds the trained model, the javascript core feature extraction, and simple
functions to apply the old or new heuristics to a web_contents*.

BUG=471439
TBR=blundell@chromium.org

Committed: https://crrev.com/adf25383033ff5794a810f725c78e319126b6edb
Cr-Commit-Position: refs/heads/master@{#323805}

[cjhopman, 2015-03-31 18:26:14]

cjhopman@chromium.org changed reviewers:
+ nyquist@chromium.org

[cjhopman, 2015-03-31 18:26:14]

nyquist: *

[nyquist, 2015-04-01 18:12:20]

https://codereview.chromium.org/1047223003/diff/1/components/components_tests...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1047223003/diff/1/components/components_tests...
components/components_tests.gyp:128:
'dom_distiller/content/distillable_page_utils_unittest.cc',
I don't think you want this.

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/content/distillable_page_utils.cc (right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/content/distillable_page_utils.cc:43: std::string
og_article_js = ResourceBundle::GetSharedInstance()
Technically IDR_IS_DISTILLABLE_JS also supports schema.org, but I guess we don't
really care about in the naming of this?

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/content/distillable_page_utils.h (right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/content/distillable_page_utils.h:15: void
IsOpenGraphArticle(content::WebContents* web_contents,
When is this supposed to be called instead of IsDistillablePage? Will we do an
A/B experiment?

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/content/distillable_page_utils_browsertest.cc
(right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/content/distillable_page_utils_browsertest.cc:25: //
dom_distiller::CalculateDerivedFeatures.
Nit: Would it be helpful to add a link to this constant from
dom_distiller::CalculateDerivedFeatures?

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/content/distillable_page_utils_browsertest.cc:129: //
The first value of the first feature is either 0 or 1. This model should
Yes, it's true that the first value is false/true (0/1 here), but the first time
I read through this I got a little bit confused. For example, why was this an
important fact, etc., probably because I'm not as used to this as you.

Anyway; I now understand it and this test makes total sense.

However, is there something we could do to make it easier for new readers? Maybe
change the wording a bit?

Not verbatim, but maybe something along the lines of:
"Since -1 as the split value will always be below both 0 and 1, the weight of 1
will always be added to the score.
The threshold for the detector will be half the total weight ( 1 / 2 = 0.5), and
that will always be below the total score of 1. Therefore this detector will
always yield a positive result."

Now when I read it it feels very verbose though, but maybe something along those
lines will help new readers.

If you end up changing anything here, please update the negative-test below as
well.

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/content/distillable_page_utils_unittest.cc
(right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/content/distillable_page_utils_unittest.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
Remove?

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/core/distillable_page_detector.cc (right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/core/distillable_page_detector.cc:48: if
(features.size() != size_t(proto_->num_features())) {
I would be OK with moving this to your previous CL and add a sanity-check style
unit test for it too. Or update the unit-test for it in this CL maybe?

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/core/javascript/extract_features.js (right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/core/javascript/extract_features.js:8:
'meta[property="og:type"],meta[name="og:type"]');
Nit: Would we need to do anything special if developers write: "OG:TYPE" instead
of "og:type"? I am assuming that's not part of the spec?

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/core/javascript/extract_features.js:18: return {
return as stringified to help out our iOS friends?

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/core/javascript/extract_features.js:25: 'textContent':
body.textContent,
It would be amazing if we at some point would get UMA stats for the calculation
of these core features. The total time would be good; and per feature would be
bestest.

[cjhopman, 2015-04-01 20:38:41]

Patchset #3 (id:40001) has been deleted

[cjhopman, 2015-04-01 20:38:46]

Patchset #2 (id:20001) has been deleted

[cjhopman, 2015-04-01 20:59:29]

https://codereview.chromium.org/1047223003/diff/1/components/components_tests...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1047223003/diff/1/components/components_tests...
components/components_tests.gyp:128:
'dom_distiller/content/distillable_page_utils_unittest.cc',
On 2015/04/01 18:12:19, nyquist wrote:
> I don't think you want this.

Done.

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/content/distillable_page_utils.cc (right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/content/distillable_page_utils.cc:43: std::string
og_article_js = ResourceBundle::GetSharedInstance()
On 2015/04/01 18:12:19, nyquist wrote:
> Technically IDR_IS_DISTILLABLE_JS also supports http://schema.org, but I guess
we don't
> really care about in the naming of this?

yeah.

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/content/distillable_page_utils.h (right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/content/distillable_page_utils.h:15: void
IsOpenGraphArticle(content::WebContents* web_contents,
On 2015/04/01 18:12:19, nyquist wrote:
> When is this supposed to be called instead of IsDistillablePage? Will we do an
> A/B experiment?

Yeah, it's not really clear here. In a bit I'll add code that actually calls
these and then it'll become more clear how these are used.

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/content/distillable_page_utils_browsertest.cc
(right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/content/distillable_page_utils_browsertest.cc:25: //
dom_distiller::CalculateDerivedFeatures.
On 2015/04/01 18:12:20, nyquist wrote:
> Nit: Would it be helpful to add a link to this constant from
> dom_distiller::CalculateDerivedFeatures?

I just moved this into that header, it makes sense there and then it's actually
by the thing it corresponds to.

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/content/distillable_page_utils_browsertest.cc:129: //
The first value of the first feature is either 0 or 1. This model should
On 2015/04/01 18:12:19, nyquist wrote:
> Yes, it's true that the first value is false/true (0/1 here), but the first
time
> I read through this I got a little bit confused. For example, why was this an
> important fact, etc., probably because I'm not as used to this as you.
> 
> Anyway; I now understand it and this test makes total sense.
> 
> However, is there something we could do to make it easier for new readers?
Maybe
> change the wording a bit?
> 
> Not verbatim, but maybe something along the lines of:
> "Since -1 as the split value will always be below both 0 and 1, the weight of
1
> will always be added to the score.
> The threshold for the detector will be half the total weight ( 1 / 2 = 0.5),
and
> that will always be below the total score of 1. Therefore this detector will
> always yield a positive result."
> 
> Now when I read it it feels very verbose though, but maybe something along
those
> lines will help new readers.
> 
> If you end up changing anything here, please update the negative-test below as
> well.
This might be better.

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/content/distillable_page_utils_unittest.cc
(right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/content/distillable_page_utils_unittest.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/04/01 18:12:20, nyquist wrote:
> Remove?

Done.

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/core/distillable_page_detector.cc (right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/core/distillable_page_detector.cc:48: if
(features.size() != size_t(proto_->num_features())) {
On 2015/04/01 18:12:20, nyquist wrote:
> I would be OK with moving this to your previous CL and add a sanity-check
style
> unit test for it too. Or update the unit-test for it in this CL maybe?

Done.

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/core/javascript/extract_features.js (right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/core/javascript/extract_features.js:8:
'meta[property="og:type"],meta[name="og:type"]');
On 2015/04/01 18:12:20, nyquist wrote:
> Nit: Would we need to do anything special if developers write: "OG:TYPE"
instead
> of "og:type"? I am assuming that's not part of the spec?

Yeah, we're not really doing this right, but it's the same way we do it in the
is_distillable_page.js.

(really we need to parse out the namespace aliases from the HEAD tag, but we
just assume that people use 'og').

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/core/javascript/extract_features.js:18: return {
On 2015/04/01 18:12:20, nyquist wrote:
> return as stringified to help out our iOS friends?

Done.

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/core/javascript/extract_features.js:25: 'textContent':
body.textContent,
On 2015/04/01 18:12:20, nyquist wrote:
> It would be amazing if we at some point would get UMA stats for the
calculation
> of these core features. The total time would be good; and per feature would be
> bestest.

Yeah, it would be nice.

[nyquist, 2015-04-01 22:30:24]

lgtm

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
File components/dom_distiller/content/distillable_page_utils_browsertest.cc
(right):

https://codereview.chromium.org/1047223003/diff/1/components/dom_distiller/co...
components/dom_distiller/content/distillable_page_utils_browsertest.cc:129: //
The first value of the first feature is either 0 or 1. This model should
On 2015/04/01 20:59:29, cjhopman wrote:
> On 2015/04/01 18:12:19, nyquist wrote:
> > Yes, it's true that the first value is false/true (0/1 here), but the first
> time
> > I read through this I got a little bit confused. For example, why was this
an
> > important fact, etc., probably because I'm not as used to this as you.
> > 
> > Anyway; I now understand it and this test makes total sense.
> > 
> > However, is there something we could do to make it easier for new readers?
> Maybe
> > change the wording a bit?
> > 
> > Not verbatim, but maybe something along the lines of:
> > "Since -1 as the split value will always be below both 0 and 1, the weight
of
> 1
> > will always be added to the score.
> > The threshold for the detector will be half the total weight ( 1 / 2 = 0.5),
> and
> > that will always be below the total score of 1. Therefore this detector will
> > always yield a positive result."
> > 
> > Now when I read it it feels very verbose though, but maybe something along
> those
> > lines will help new readers.
> > 
> > If you end up changing anything here, please update the negative-test below
as
> > well.
> This might be better.

Yes. Thanks!

https://codereview.chromium.org/1047223003/diff/80001/components/dom_distille...
File components/dom_distiller/core/distillable_page_detector.h (right):

https://codereview.chromium.org/1047223003/diff/80001/components/dom_distille...
components/dom_distiller/core/distillable_page_detector.h:25: // Returns true if
the model classifies the vector of features as a
Nit: Returns whether the model cl...

[cjhopman, 2015-04-03 16:57:04]

The CQ bit was checked by cjhopman@chromium.org

[cjhopman, 2015-04-03 16:57:04]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1047223003/#ps100001
(title: "GN")

[commit-bot: I haz the power, 2015-04-03 16:57:31]

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

[commit-bot: I haz the power, 2015-04-03 17:03:25]

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

[commit-bot: I haz the power, 2015-04-03 17:03:25]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[cjhopman, 2015-04-03 18:36:40]

cjhopman@chromium.org changed reviewers:
+ blundell@chromium.org

[cjhopman, 2015-04-03 18:36:42]

blundell TBRed for the components/BUILD.gn change

[cjhopman, 2015-04-03 18:37:04]

The CQ bit was checked by cjhopman@chromium.org

[cjhopman, 2015-04-03 18:37:04]

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

[commit-bot: I haz the power, 2015-04-03 18:37:27]

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

[commit-bot: I haz the power, 2015-04-03 19:10:17]

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

[commit-bot: I haz the power, 2015-04-03 19:10:18]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[cjhopman, 2015-04-03 19:13:11]

The CQ bit was checked by cjhopman@chromium.org

[cjhopman, 2015-04-03 19:13:12]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1047223003/#ps140001
(title: "whitelist resources for ios")

[commit-bot: I haz the power, 2015-04-03 19:13:28]

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

[commit-bot: I haz the power, 2015-04-03 20:29:11]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-04-03 20:40:42]

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

[blundell, 2015-04-07 07:21:30]

lgtm