Make DistillerPageWebContentsTest a real test that uses the distiller.

Make DistillerPageWebContentsTest a real test that uses the distiller.

Previously, the test used a hardcoded distiller template which didn't
help test the distiller. Updates the test to use the real distiller and
adds tests for relativization of images and anchors.

In the process, attempts to handle a few failure modes of the distiller.

BUG=353349, 354737
TBR=blundell

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

[Yaron, 2014-03-26 02:29:37]

I think I'll still add a few more tests (e.g. page that fails distillation
entirely) but thought to get this out for a first look.

[kuan, 2014-03-26 13:07:48]

https://codereview.chromium.org/211493008/diff/1/components/dom_distiller/con...
File components/dom_distiller/content/distiller_page_web_contents_browsertest.cc
(right):

https://codereview.chromium.org/211493008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/distiller_page_web_contents_browsertest.cc:46:
.as_string();
i think according to styling guidelines, '.' should go on the previous line?

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

https://codereview.chromium.org/211493008/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/page_distiller.cc:97: if
(!unvalidated_gurl.is_empty() && unvalidated_gurl.is_valid()) {
no need to check for is_empty; as per gurl.h, empty urls are also invalid.

[kuan, 2014-03-26 16:48:23]

oh, i forgot.  it's lgtm if u're not adding anymore tests in this cl.

[Yaron, 2014-03-26 17:31:50]

Thanks. Actually, I realized that handling a complete failure would be a
page_distiller test so I'll stick with this for the time being.

cjhopman: owners
blundell: tbr for components/components_tests.gyp

https://codereview.chromium.org/211493008/diff/1/components/dom_distiller/con...
File components/dom_distiller/content/distiller_page_web_contents_browsertest.cc
(right):

https://codereview.chromium.org/211493008/diff/1/components/dom_distiller/con...
components/dom_distiller/content/distiller_page_web_contents_browsertest.cc:46:
.as_string();
On 2014/03/26 13:07:48, kuan wrote:
> i think according to styling guidelines, '.' should go on the previous line?

Done.

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

https://codereview.chromium.org/211493008/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/page_distiller.cc:97: if
(!unvalidated_gurl.is_empty() && unvalidated_gurl.is_valid()) {
On 2014/03/26 13:07:48, kuan wrote:
> no need to check for is_empty; as per gurl.h, empty urls are also invalid.

Done.

[blundell, 2014-03-26 21:00:00]

components_tests.gyp LGTM

[cjhopman, 2014-03-27 00:22:39]

lgtm

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

https://codereview.chromium.org/211493008/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/page_distiller.cc:97: if
(!unvalidated_gurl.is_empty() && unvalidated_gurl.is_valid()) {
On 2014/03/26 17:31:50, Yaron wrote:
> On 2014/03/26 13:07:48, kuan wrote:
> > no need to check for is_empty; as per gurl.h, empty urls are also invalid.
> 
> Done.

Is a patchset missing? This is still checking is_empty

https://codereview.chromium.org/211493008/diff/60001/components/dom_distiller...
File components/dom_distiller/content/distiller_page_web_contents_browsertest.cc
(right):

https://codereview.chromium.org/211493008/diff/60001/components/dom_distiller...
components/dom_distiller/content/distiller_page_web_contents_browsertest.cc:124:
ContainsRegex("href=\"http://127.0.0.1:.*/relativelink.html\""));
Can we make the ".*" a bit stricter (like [0-9]*)? Will it always have a port
number? Same below.

https://codereview.chromium.org/211493008/diff/60001/components/dom_distiller...
File components/dom_distiller/core/javascript/domdistiller.js (right):

https://codereview.chromium.org/211493008/diff/60001/components/dom_distiller...
components/dom_distiller/core/javascript/domdistiller.js:23:
window.console.log("Erring during distillation: " + e);
s/Erring/Error ?

[Yaron, 2014-04-02 17:11:58]

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

https://codereview.chromium.org/211493008/diff/1/components/dom_distiller/cor...
components/dom_distiller/core/page_distiller.cc:97: if
(!unvalidated_gurl.is_empty() && unvalidated_gurl.is_valid()) {
On 2014/03/27 00:22:39, cjhopman wrote:
> On 2014/03/26 17:31:50, Yaron wrote:
> > On 2014/03/26 13:07:48, kuan wrote:
> > > no need to check for is_empty; as per gurl.h, empty urls are also invalid.
> > 
> > Done.
> 
> Is a patchset missing? This is still checking is_empty

Done.

https://codereview.chromium.org/211493008/diff/60001/components/dom_distiller...
File components/dom_distiller/content/distiller_page_web_contents_browsertest.cc
(right):

https://codereview.chromium.org/211493008/diff/60001/components/dom_distiller...
components/dom_distiller/content/distiller_page_web_contents_browsertest.cc:124:
ContainsRegex("href=\"http://127.0.0.1:.*/relativelink.html\""));
On 2014/03/27 00:22:39, cjhopman wrote:
> Can we make the ".*" a bit stricter (like [0-9]*)? Will it always have a port
> number? Same below.

Done.

https://codereview.chromium.org/211493008/diff/60001/components/dom_distiller...
File components/dom_distiller/core/javascript/domdistiller.js (right):

https://codereview.chromium.org/211493008/diff/60001/components/dom_distiller...
components/dom_distiller/core/javascript/domdistiller.js:23:
window.console.log("Erring during distillation: " + e);
On 2014/03/27 00:22:39, cjhopman wrote:
> s/Erring/Error ?

Done.

[Yaron, 2014-04-02 17:12:02]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-04-02 17:13:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/211493008/80001

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

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

[commit-bot: I haz the power, 2014-04-02 17:19:37]

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

[Yaron, 2014-04-02 20:38:10]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-04-02 20:39:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/211493008/80001

[commit-bot: I haz the power, 2014-04-02 20:48:04]

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

[commit-bot: I haz the power, 2014-04-02 20:48:04]

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

[Yaron, 2014-04-03 16:03:40]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-04-03 16:06:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/211493008/80001

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

Change committed as 261463