Add support for providing an external file for extracting content.

Add support for providing an external file for extracting content.

This CL adds the command line parameter --external-dom-distiller-js
to the content extractor used in DOM Distiller extraction tests.

By providing this parameter, the user can provide her own version of
the JavaScript instead of using the built-in one. This removes the
requirement of re-compiling the components_browsertests target
when iterating on DOM Distiller JavaScript code.

Requires these changes from DOM Distiller:
https://codereview.chromium.org/863863007/

BUG=455503

[nyquist, 2015-02-05 01:41:08]

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

[nyquist, 2015-02-05 01:41:09]

cjhopman: PTAL

[cjhopman, 2015-02-05 02:09:40]

https://codereview.chromium.org/901793002/diff/1/components/dom_distiller/cor...
File components/dom_distiller/core/distiller_page.cc (right):

https://codereview.chromium.org/901793002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/distiller_page.cc:32: if
(script_with_options.empty()) {
this could just check script.empty() rather than working on the copy

https://codereview.chromium.org/901793002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/distiller_page.cc:46: script_with_options =
script_with_options.replace(
I'd move the copy down here where it's needed.

https://codereview.chromium.org/901793002/diff/1/components/dom_distiller/cor...
File components/dom_distiller/core/distiller_page.h (right):

https://codereview.chromium.org/901793002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/distiller_page.h:62: // An optional script for
overriding the JS that comes packaged with Chrome.
This isn't really optional, it will always hold the script to use

[nyquist, 2015-02-06 18:50:32]

cjhopman: PTAL

https://codereview.chromium.org/901793002/diff/1/components/dom_distiller/cor...
File components/dom_distiller/core/distiller_page.cc (right):

https://codereview.chromium.org/901793002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/distiller_page.cc:32: if
(script_with_options.empty()) {
On 2015/02/05 02:09:40, cjhopman wrote:
> this could just check script.empty() rather than working on the copy

Done.

https://codereview.chromium.org/901793002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/distiller_page.cc:46: script_with_options =
script_with_options.replace(
On 2015/02/05 02:09:40, cjhopman wrote:
> I'd move the copy down here where it's needed.

Done.

https://codereview.chromium.org/901793002/diff/1/components/dom_distiller/cor...
File components/dom_distiller/core/distiller_page.h (right):

https://codereview.chromium.org/901793002/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/distiller_page.h:62: // An optional script for
overriding the JS that comes packaged with Chrome.
On 2015/02/05 02:09:40, cjhopman wrote:
> This isn't really optional, it will always hold the script to use

Done.
Also updated the comment above the constructor to clarify the need for
$$OPTIONS.

[cjhopman, 2015-02-07 01:16:48]

https://codereview.chromium.org/901793002/diff/20001/components/dom_distiller...
File components/dom_distiller/content/distiller_page_web_contents.h (right):

https://codereview.chromium.org/901793002/diff/20001/components/dom_distiller...
components/dom_distiller/content/distiller_page_web_contents.h:48: // An
optional script for overriding the JS that comes packaged with Chrome.
I'd say make this non-optional and have the caller supply the one in the pak
file if that's what it wants (and then make it required int the DistillerPage
constructors too).

https://codereview.chromium.org/901793002/diff/20001/components/dom_distiller...
components/dom_distiller/content/distiller_page_web_contents.h:56:
DistillerPageWebContents(
shouldn't this constructor take the distiller script?

[nyquist, 2015-03-09 20:40:03]

cjhopman: PTAL

This depends on a roll of distiller, so all tryjobs will fail.

https://codereview.chromium.org/901793002/diff/20001/components/dom_distiller...
File components/dom_distiller/content/distiller_page_web_contents.h (right):

https://codereview.chromium.org/901793002/diff/20001/components/dom_distiller...
components/dom_distiller/content/distiller_page_web_contents.h:48: // An
optional script for overriding the JS that comes packaged with Chrome.
On 2015/02/07 01:16:48, cjhopman wrote:
> I'd say make this non-optional and have the caller supply the one in the pak
> file if that's what it wants (and then make it required int the DistillerPage
> constructors too).

Done.

https://codereview.chromium.org/901793002/diff/20001/components/dom_distiller...
components/dom_distiller/content/distiller_page_web_contents.h:56:
DistillerPageWebContents(
On 2015/02/07 01:16:48, cjhopman wrote:
> shouldn't this constructor take the distiller script?

Done.

[nyquist, 2015-03-11 14:12:58]

noyau: PTAL ios/, components/dom_distiller/ios

[nyquist, 2015-03-11 14:14:38]

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

[nyquist, 2015-03-11 14:14:39]

noyau: PTAL ios/, components/dom_distiller/ios

This time actually adding noyau@...

[noyau (Ping after 24h), 2015-03-11 14:31:00]

https://codereview.chromium.org/901793002/diff/40001/components/dom_distiller...
File components/dom_distiller/core/distiller_page.h (right):

https://codereview.chromium.org/901793002/diff/40001/components/dom_distiller...
components/dom_distiller/core/distiller_page.h:59: virtual void
DistillPageImpl(const GURL& url, const std::string& script) = 0;
The comment should specify that the options have been replaced in the passed on
script.

https://codereview.chromium.org/901793002/diff/40001/components/resources/dom...
File components/resources/dom_distiller_resources.grdp (right):

https://codereview.chromium.org/901793002/diff/40001/components/resources/dom...
components/resources/dom_distiller_resources.grdp:9: <include
name="IDR_DISTILLER_JS"
file="../../third_party/dom_distiller_js/package/js/domdistiller_wrapped.js"
type="BINDATA" />
I'm not for that change. This file needs iOS specific changes that are going to
be hard to maintain: now the ios file and the non ios file are in two different
repositories.

Instead I made a CL (https://codereview.chromium.org/995343002/) that merge the
two files in one and uses grit <if> to do the right thing, diminishing the
maintenance burden.

[cjhopman, 2015-03-12 02:03:14]

https://codereview.chromium.org/901793002/diff/40001/components/resources/dom...
File components/resources/dom_distiller_resources.grdp (right):

https://codereview.chromium.org/901793002/diff/40001/components/resources/dom...
components/resources/dom_distiller_resources.grdp:9: <include
name="IDR_DISTILLER_JS"
file="../../third_party/dom_distiller_js/package/js/domdistiller_wrapped.js"
type="BINDATA" />
On 2015/03/11 14:30:59, noyau wrote:
> I'm not for that change. This file needs iOS specific changes that are going
to
> be hard to maintain: now the ios file and the non ios file are in two
different
> repositories.
> 
> Instead I made a CL (https://codereview.chromium.org/995343002/) that merge
the
> two files in one and uses grit <if> to do the right thing, diminishing the
> maintenance burden.

Yeah, we should definitely merge these into one file. We would like to maintain
this in the dom distiller repo because the file basically needs to stay in sync
with the api exposed there (but doesn't need to change for chrome-side changes).

noyau, are you particularly opposed to it being maintained in the other repo? my
impression is that you mostly just don't want us to be maintaining two similar,
but different, versions of it. If it's in the other repo, we would also need to
support different platforms via javascript stuff rather than grit stuff. wdyt?

[noyau (Ping after 24h), 2015-03-12 09:17:30]

On 2015/03/12 02:03:14, cjhopman wrote:
>
https://codereview.chromium.org/901793002/diff/40001/components/resources/dom...
> File components/resources/dom_distiller_resources.grdp (right):
> 
>
https://codereview.chromium.org/901793002/diff/40001/components/resources/dom...
> components/resources/dom_distiller_resources.grdp:9: <include
> name="IDR_DISTILLER_JS"
> file="../../third_party/dom_distiller_js/package/js/domdistiller_wrapped.js"
> type="BINDATA" />
> On 2015/03/11 14:30:59, noyau wrote:
> > I'm not for that change. This file needs iOS specific changes that are going
> to
> > be hard to maintain: now the ios file and the non ios file are in two
> different
> > repositories.
> > 
> > Instead I made a CL (https://codereview.chromium.org/995343002/) that merge
> the
> > two files in one and uses grit <if> to do the right thing, diminishing the
> > maintenance burden.
> 
> Yeah, we should definitely merge these into one file. We would like to
maintain
> this in the dom distiller repo because the file basically needs to stay in
sync
> with the api exposed there (but doesn't need to change for chrome-side
changes).
> 
> noyau, are you particularly opposed to it being maintained in the other repo?
my
> impression is that you mostly just don't want us to be maintaining two
similar,
> but different, versions of it. If it's in the other repo, we would also need
to
> support different platforms via javascript stuff rather than grit stuff. wdyt?

No I'm not opposed to moving the wrapper into the other repository. I just don't
want two separate files to maintain. I'm just curious how the platform detection
will be done in javascript, do you have a plan?

I would argue however that this should be a separate CL:

* This CL should only deal with what its title describes to: allowing a
command-line option for the browser tests.
* Then landing my CL to move to only use one js file
* And only then make another CL moving the js file into domdistiller and reving
domdistiller at the same time

This will allow keeping everything working through the transition. Landing this
as is is very likely going to break iOS in the process.