Use boilerpipe-based dom-distiller-js

Use boilerpipe-based dom-distiller-js

This changes the dom distiller to use the boilerpipe-based
dom-distiller-js library.

Adds a simple script to update the compiled domdistiller.js file in
third_party.

TBR=blundell@chromium.org
BUG=288015
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257499

[nyquist, 2014-02-26 00:30:15]

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller/cor...
File components/dom_distiller/core/javascript/domdistiller.js (right):

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/javascript/domdistiller.js:5: <include
src="../../../../third_party/dom_distiller_js/js/domdistiller.js"/>
To be honest, I didn't know
RenderViewHost::ExecuteJavascriptInWebFrameCallbackResult supported HTML as
well...

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
File third_party/dom_distiller_js/LICENSE (right):

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/LICENSE:1: copyright (c) 2014, The Chromium Authors
Should there be a capital C?

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
File third_party/dom_distiller_js/README.chromium (right):

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/README.chromium:8: Dom Distiller is a java library
that extracts article content from a web page.
Could you add a description as to how to update this?

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
File third_party/dom_distiller_js/update_domdistiller_js.sh (right):

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/update_domdistiller_js.sh:12:
compiled_js_path=third_party/dom_distiller_js/js/domdistiller.js
Does this require running the script from src/? If so, document it above.

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/update_domdistiller_js.sh:20: ant extractjs
Does all checkouts have ant? Otherwise, could you list the requirement for ant
somewhere?

[nyquist, 2014-02-26 00:53:15]

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller/cor...
File components/dom_distiller/core/javascript/domdistiller.js (right):

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/javascript/domdistiller.js:5: <include
src="../../../../third_party/dom_distiller_js/js/domdistiller.js"/>
On 2014/02/26 00:30:15, nyquist wrote:
> To be honest, I didn't know
> RenderViewHost::ExecuteJavascriptInWebFrameCallbackResult supported HTML as
> well...

In fact, since this is GRIT, could we maybe add a short comment in these two
files for new readers? Since the file extension is .js it's a little bit
confusing.

[Yaron, 2014-02-26 02:50:49]

Add BUG=288015

Should also follow http://www.chromium.org/developers/adding-3rd-party-libraries
and get quick sign-off. Chrome Eng Review seems unwarranted as we're just
changing the underlying library but the design remains the same. Same goes for
security review as we're already insulated from this in the same way but can ask
tsepez whether to re-review

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller/cor...
File components/dom_distiller/core/javascript/domdistiller.js (right):

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/javascript/domdistiller.js:14: readability.init();
Add TODO: remove dependency on readability.

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller_res...
File components/dom_distiller_resources.grdp (right):

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller_res...
components/dom_distiller_resources.grdp:8: <include name="IDR_DISTILLER_JS"
file="dom_distiller/core/javascript/domdistiller.js" flattenhtml="true"
allowexternalscript="true" type="BINDATA" />
Does this end up in us having two copies of readability? One in the boilerpipe
version (temporary) and the one that's currently in use? If so, can you make a
gyp flag to control which one is included?

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
File third_party/dom_distiller_js/LICENSE (right):

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/LICENSE:1: copyright (c) 2014, The Chromium Authors
On 2014/02/26 00:30:15, nyquist wrote:
> Should there be a capital C?

Also no "(c)" is the new hotness :)

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/LICENSE:1: copyright (c) 2014, The Chromium Authors
This dir should have an OWNERS file with us.

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
File third_party/dom_distiller_js/README.chromium (right):

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/README.chromium:4: License: BSD
Actually, why did you create this as BSD? Is it valid? The original library was
Apache2.

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
File third_party/dom_distiller_js/update_domdistiller_js.sh (right):

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/update_domdistiller_js.sh:20: ant extractjs
On 2014/02/26 00:30:15, nyquist wrote:
> Does all checkouts have ant? Otherwise, could you list the requirement for ant
> somewhere?

No they don't. Only android checkouts have it so +1.

[cjhopman, 2014-02-27 02:43:09]

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller/cor...
File components/dom_distiller/core/javascript/domdistiller.js (right):

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/javascript/domdistiller.js:5: <include
src="../../../../third_party/dom_distiller_js/js/domdistiller.js"/>
On 2014/02/26 00:53:16, nyquist wrote:
> On 2014/02/26 00:30:15, nyquist wrote:
> > To be honest, I didn't know
> > RenderViewHost::ExecuteJavascriptInWebFrameCallbackResult supported HTML as
> > well...
> 
> In fact, since this is GRIT, could we maybe add a short comment in these two
> files for new readers? Since the file extension is .js it's a little bit
> confusing.

Done.

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/javascript/domdistiller.js:14: readability.init();
On 2014/02/26 02:50:50, Yaron wrote:
> Add TODO: remove dependency on readability.

Done.

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller_res...
File components/dom_distiller_resources.grdp (right):

https://codereview.chromium.org/176083002/diff/1/components/dom_distiller_res...
components/dom_distiller_resources.grdp:8: <include name="IDR_DISTILLER_JS"
file="dom_distiller/core/javascript/domdistiller.js" flattenhtml="true"
allowexternalscript="true" type="BINDATA" />
On 2014/02/26 02:50:50, Yaron wrote:
> Does this end up in us having two copies of readability? One in the boilerpipe
> version (temporary) and the one that's currently in use? If so, can you make a
> gyp flag to control which one is included?

Yeah. I've just removed this one. If we want to run eval with readability, we
can just add a separate set of resources for the standalone content_extractor.

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
File third_party/dom_distiller_js/LICENSE (right):

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/LICENSE:1: copyright (c) 2014, The Chromium Authors
On 2014/02/26 02:50:50, Yaron wrote:
> This dir should have an OWNERS file with us. 

Done.

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/LICENSE:1: copyright (c) 2014, The Chromium Authors
On 2014/02/26 02:50:50, Yaron wrote:
> On 2014/02/26 00:30:15, nyquist wrote:
> > Should there be a capital C?
> 
> Also no "(c)" is the new hotness :)

Done.

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/LICENSE:1: copyright (c) 2014, The Chromium Authors
On 2014/02/26 00:30:15, nyquist wrote:
> Should there be a capital C?

Done.

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
File third_party/dom_distiller_js/README.chromium (right):

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/README.chromium:4: License: BSD
On 2014/02/26 02:50:50, Yaron wrote:
> Actually, why did you create this as BSD? Is it valid? The original library
was
> Apache2.
My understanding: BSD is the default Chromium license and so the dom-distiller
project (on google code) uses the BSD license. Parts of that project
(particularly boilerpipe-core) are also available under other licenses (i.e.
Apache 2.0).

This change requires a LICENSE file in this folder and that will basically say
what I've just said a above (in many more words).

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/README.chromium:8: Dom Distiller is a java library
that extracts article content from a web page.
On 2014/02/26 00:30:15, nyquist wrote:
> Could you add a description as to how to update this?

Done.

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
File third_party/dom_distiller_js/update_domdistiller_js.sh (right):

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/update_domdistiller_js.sh:12:
compiled_js_path=third_party/dom_distiller_js/js/domdistiller.js
On 2014/02/26 00:30:15, nyquist wrote:
> Does this require running the script from src/? If so, document it above.

Done.

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/update_domdistiller_js.sh:20: ant extractjs
On 2014/02/26 00:30:15, nyquist wrote:
> Does all checkouts have ant? Otherwise, could you list the requirement for ant
> somewhere?

Done.

[nyquist, 2014-03-03 18:08:56]

lgtm

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
File third_party/dom_distiller_js/README.chromium (right):

https://codereview.chromium.org/176083002/diff/1/third_party/dom_distiller_js...
third_party/dom_distiller_js/README.chromium:4: License: BSD
On 2014/02/27 02:43:09, cjhopman wrote:
> On 2014/02/26 02:50:50, Yaron wrote:
> > Actually, why did you create this as BSD? Is it valid? The original library
> was
> > Apache2.
> My understanding: BSD is the default Chromium license and so the dom-distiller
> project (on google code) uses the BSD license. Parts of that project
> (particularly boilerpipe-core) are also available under other licenses (i.e.
> Apache 2.0).
> 
> This change requires a LICENSE file in this folder and that will basically say
> what I've just said a above (in many more words).

For new readers, see: https://codereview.chromium.org/177283005/

https://codereview.chromium.org/176083002/diff/50001/third_party/dom_distille...
File third_party/dom_distiller_js/README.chromium (right):

https://codereview.chromium.org/176083002/diff/50001/third_party/dom_distille...
third_party/dom_distiller_js/README.chromium:12: Run update_domdistiller_js.sh.
You have to open up the script itself to see that it requires to be run from
src/. Could you be a bit more clear here about that for now? And maybe a
super-quick comment that this requires ant?

[Yaron, 2014-03-05 19:40:43]

code lgtm but requires the follow-up I requested previously

[cjhopman, 2014-03-07 01:39:18]

tsepez: For some security review. This is a replacement for the readability.js
that we have been temporarily using.

Basically it follows the same pattern as suggested in the review of
readability-- just clone the nodes and never set the inner HTML of anything.

I don't know how detailed you want to get, and it's not really particularly
reviewable here. The full repo (with the relevant source) is at
https://code.google.com/p/dom-distiller/source/browse/

All the files that actually access DOM nodes in any way are in a single folder:
https://code.google.com/p/dom-distiller/source/browse/src/com/dom_distiller/c...
xsrf_token: 99f23a9b5e7bc172aad2a54b8d745281
send_mail: on
subject: Use boilerpipe-based dom-distiller-js

[cjhopman, 2014-03-13 17:34:53]

tsepez: ping

[Tom Sepez, 2014-03-13 18:25:38]

On 2014/03/13 17:34:53, cjhopman wrote:
> tsepez: ping

As you say, this isn't really reviewable from a security standpoint, and I worry
about the maintenance burden of relying on the Java -> JS translation. Are there
other parts of chrome that are using this mechanism?

[Tom Sepez, 2014-03-13 19:17:07]

> As you say, this isn't really reviewable from a security standpoint, and I
worry
> about the maintenance burden of relying on the Java -> JS translation. Are
there
> other parts of chrome that are using this mechanism?
Alternatively, is it possible to get GWT to spit out a non-minimized
human-readable form for review?

[cjhopman, 2014-03-13 20:43:16]

On 2014/03/13 19:17:07, Tom Sepez wrote:
> > As you say, this isn't really reviewable from a security standpoint, and 
> Alternatively, is it possible to get GWT to spit out a non-minimized
> human-readable form for review?

Sure, I've changed the update script to use gwt's PRETTY output. It's ~150k
instead of ~50k, but is at least sort of readable.

> I
> worry
> > about the maintenance burden of relying on the Java -> JS translation. Are
> there
> > other parts of chrome that are using this mechanism?

I worry about this, too (and no, other parts of chrome don't use it). I really
think that we will eventually rewrite this in javascript, but we don't want to
block progress on prototyping and testing on such a rewrite (and such testing
might convince us that we don't want this at all).

[Tom Sepez, 2014-03-14 00:29:34]

Ok, I didn't see any obvious red flags in a cursory reading of the unminified
code.  LGTM.

[cjhopman, 2014-03-15 00:05:15]

And now adding in open-source-third-party-reviews@google.com

[Daniel Berlin, 2014-03-15 01:50:57]

LGTM for third party licenses

On Fri, Mar 14, 2014 at 5:05 PM,  <cjhopman@chromium.org> wrote:
> And now adding in open-source-third-party-reviews@google.com
>
> https://codereview.chromium.org/176083002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cjhopman, 2014-03-17 16:44:34]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-03-17 16:44:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/176083002/120001

[commit-bot: I haz the power, 2014-03-17 16:55:47]

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

[commit-bot: I haz the power, 2014-03-17 16:55:48]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_compile_dbg

[cjhopman, 2014-03-17 17:08:02]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-03-17 17:08:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/176083002/130010

[commit-bot: I haz the power, 2014-03-17 17:36:03]

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

[commit-bot: I haz the power, 2014-03-17 17:36:04]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[cjhopman, 2014-03-17 17:41:32]

blundell@chromium.org: Please review changes in components/resources

[cjhopman, 2014-03-17 18:06:56]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-03-17 18:07:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/176083002/150001

[commit-bot: I haz the power, 2014-03-17 18:10:30]

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

[commit-bot: I haz the power, 2014-03-17 18:10:31]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg
  tryserver.chromium on linux_chromium_chromeos_rel
  tryserver.chromium on linux_chromium_clang_dbg
  tryserver.chromium on linux_chromium_rel
  tryserver.chromium on mac_chromium_compile_dbg
  tryserver.chromium on mac_chromium_rel
  tryserver.chromium on win_chromium_compile_dbg

[cjhopman, 2014-03-17 18:36:16]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-03-17 18:36:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/176083002/150001

[commit-bot: I haz the power, 2014-03-17 19:54:09]

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

[commit-bot: I haz the power, 2014-03-17 19:54:10]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[cjhopman, 2014-03-17 20:01:15]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-03-17 20:01:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/176083002/150001

[commit-bot: I haz the power, 2014-03-17 21:40:44]

Change committed as 257499

[blundell, 2014-03-18 09:44:47]

components/resources LGTM