JavaScript that returns whether or not a page is an article

JavaScript that returns whether or not a page is an article

Has a JavaScript file that contains a function to tell whether or not a
page is an article and related functions to get a string of that JavaScript
so that it can be injected into the page.

BUG=383630
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289192

[Yaron, 2014-08-12 18:55:06]

https://codereview.chromium.org/464973002/diff/1/components/dom_distiller/and...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerUrlUtils.java
(right):

https://codereview.chromium.org/464973002/diff/1/components/dom_distiller/and...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerUrlUtils.java:52:
public static String getIsReadableJs() {
Please rename "readable"->"distillable" throughout for consistency for the read
of the codebase.
add javadoc as well.
Also please move to DomDistillerService. 

Aside: this is probably better as a function of DomDistillerService. Rather than
exporting the JS, it could be a queryable service. The problem I see with doing
that right now is that this lives in dom_distiller/core so you'd need to be
based on SourcePageHandle which isn't in java. I guess you can TODO(yfriedman)
that.

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

https://codereview.chromium.org/464973002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/javascript/is_readable.js:1: function() {
Add copyright

https://codereview.chromium.org/464973002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/javascript/is_readable.js:1: function() {
Rename something like is_distillable_trigger

[smaslo, 2014-08-12 20:42:24]

nyquist:
Just an FYI, Yaron is not an owner for these files so please eventually provide
owner approval.

https://codereview.chromium.org/464973002/diff/1/components/dom_distiller/and...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerUrlUtils.java
(right):

https://codereview.chromium.org/464973002/diff/1/components/dom_distiller/and...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerUrlUtils.java:52:
public static String getIsReadableJs() {
On 2014/08/12 18:55:05, Yaron wrote:
> Please rename "readable"->"distillable" throughout for consistency for the
read
> of the codebase.
> add javadoc as well.
> Also please move to DomDistillerService. 
> 
> Aside: this is probably better as a function of DomDistillerService. Rather
than
> exporting the JS, it could be a queryable service. The problem I see with
doing
> that right now is that this lives in dom_distiller/core so you'd need to be
> based on SourcePageHandle which isn't in java. I guess you can TODO(yfriedman)
> that.

Done.

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

https://codereview.chromium.org/464973002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/javascript/is_readable.js:1: function() {
I renamed the class.  If I give the method a name, then I would have to invoke
it rather than executing it automatically.
On 2014/08/12 18:55:05, Yaron wrote:
> Rename something like is_distillable_trigger

https://codereview.chromium.org/464973002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/javascript/is_readable.js:1: function() {
On 2014/08/12 18:55:05, Yaron wrote:
> Add copyright

Done.

[Yaron, 2014-08-12 20:49:00]

lg with those changes but as you said, needs nyquist

https://codereview.chromium.org/464973002/diff/20001/components/dom_distiller...
File components/dom_distiller/core/javascript/is_distillable_trigger.js (right):

https://codereview.chromium.org/464973002/diff/20001/components/dom_distiller...
components/dom_distiller/core/javascript/is_distillable_trigger.js:5: function()
{
Why did you remove the closure wrap? It's no longer self-executing and also
exposed to the original page.

https://codereview.chromium.org/464973002/diff/20001/components/dom_distiller...
File components/dom_distiller/core/url_utils.h (right):

https://codereview.chromium.org/464973002/diff/20001/components/dom_distiller...
components/dom_distiller/core/url_utils.h:34: // Returns the JavaScript to tell
whether or not a page is an article.
s/an article/is likely usable with DomDistillerService/

err this should probably also describe the expectations of the returned snippet.
Something like: "Returns a snippet of javascript that can be executed in a live
page and will return a boolean if the page should be used with
DomDistillerService"

[smaslo, 2014-08-12 20:58:15]

https://codereview.chromium.org/464973002/diff/20001/components/dom_distiller...
File components/dom_distiller/core/javascript/is_distillable_trigger.js (right):

https://codereview.chromium.org/464973002/diff/20001/components/dom_distiller...
components/dom_distiller/core/javascript/is_distillable_trigger.js:5: function()
{
On 2014/08/12 20:48:59, Yaron wrote:
> Why did you remove the closure wrap? It's no longer self-executing and also
> exposed to the original page.

Done.

https://codereview.chromium.org/464973002/diff/20001/components/dom_distiller...
File components/dom_distiller/core/url_utils.h (right):

https://codereview.chromium.org/464973002/diff/20001/components/dom_distiller...
components/dom_distiller/core/url_utils.h:34: // Returns the JavaScript to tell
whether or not a page is an article.
On 2014/08/12 20:49:00, Yaron wrote:
> s/an article/is likely usable with DomDistillerService/
> 
> err this should probably also describe the expectations of the returned
snippet.
> Something like: "Returns a snippet of javascript that can be executed in a
live
> page and will return a boolean if the page should be used with
> DomDistillerService"

Done.

[nyquist, 2014-08-12 21:54:15]

lgtm

[smaslo, 2014-08-12 21:57:00]

The CQ bit was checked by smaslo@chromium.org

[commit-bot: I haz the power, 2014-08-12 22:03:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/smaslo@chromium.org/464973002/40001

[commit-bot: I haz the power, 2014-08-13 00:33:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

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

Change committed as 289192