Show template before distiller finishes

Show template before distiller finishes

This change allows content to be shown on the page prior to the
distillation of the first page of an article having completed.
The template HTML and the loading animation will show while the
distillation completes.

BUG=

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

[mdjones, 2015-05-12 22:54:44]

mdjones@chromium.org changed reviewers:
+ cjhopman@chromium.org, noyau@chromium.org

[mdjones, 2015-05-12 22:54:45]

This change does not include 'correctly' running javascript on iOS.

[wychen, 2015-05-15 01:36:13]

wychen@chromium.org changed reviewers:
+ wychen@chromium.org

[wychen, 2015-05-15 01:36:14]

https://codereview.chromium.org/1130703003/diff/80001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_request_view_base_unittest.cc
(right):

https://codereview.chromium.org/1130703003/diff/80001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base_unittest.cc:33:
TEST_F(DomDistillerRequestViewTest, TestTitleNeverEmpty) {
Maybe also check special char to see if the title is escaped correctly?

[mdjones, 2015-05-18 15:33:33]

https://codereview.chromium.org/1130703003/diff/80001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_request_view_base_unittest.cc
(right):

https://codereview.chromium.org/1130703003/diff/80001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base_unittest.cc:33:
TEST_F(DomDistillerRequestViewTest, TestTitleNeverEmpty) {
On 2015/05/15 01:36:14, wychen wrote:
> Maybe also check special char to see if the title is escaped correctly?

Done.

[noyau (Ping after 24h), 2015-05-20 16:29:23]

https://codereview.chromium.org/1130703003/diff/120001/components/dom_distill...
File components/dom_distiller/core/BUILD.gn (right):

https://codereview.chromium.org/1130703003/diff/120001/components/dom_distill...
components/dom_distiller/core/BUILD.gn:101: "test_request_view_handle.h",
If you want those tests to run on ios, you need to add this file to the gyp
files as well

[mdjones, 2015-05-20 20:27:29]

https://codereview.chromium.org/1130703003/diff/120001/components/dom_distill...
File components/dom_distiller/core/BUILD.gn (right):

https://codereview.chromium.org/1130703003/diff/120001/components/dom_distill...
components/dom_distiller/core/BUILD.gn:101: "test_request_view_handle.h",
On 2015/05/20 16:29:23, noyau wrote:
> If you want those tests to run on ios, you need to add this file to the gyp
> files as well

Done.

[noyau (Ping after 24h), 2015-05-21 08:24:56]

lgtm

[cjhopman, 2015-05-21 20:53:45]

https://codereview.chromium.org/1130703003/diff/140001/chrome/browser/dom_dis...
File chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc
(right):

https://codereview.chromium.org/1130703003/diff/140001/chrome/browser/dom_dis...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:273: //
Setup observer to inspect the RenderViewHost after committed navigation.
This comment doesn't make sense to me we aren't setting up an observer here.

Also, why not get this just before it's actually used?

https://codereview.chromium.org/1130703003/diff/140001/components/dom_distill...
File components/dom_distiller/core/dom_distiller_request_view_base.cc (right):

https://codereview.chromium.org/1130703003/diff/140001/components/dom_distill...
components/dom_distiller/core/dom_distiller_request_view_base.cc:55: std::string
title(article_proto->title());
why not just inline this in the call below? we only use it that once, right?

https://codereview.chromium.org/1130703003/diff/140001/components/dom_distill...
components/dom_distiller/core/dom_distiller_request_view_base.cc:90: std::string
title(page.title());
same as above

https://codereview.chromium.org/1130703003/diff/140001/components/dom_distill...
File components/dom_distiller/core/html/dom_distiller_viewer.html (right):

https://codereview.chromium.org/1130703003/diff/140001/components/dom_distill...
components/dom_distiller/core/html/dom_distiller_viewer.html:11: <title></title>
Should we have a title of some sort while loading?

https://codereview.chromium.org/1130703003/diff/140001/components/dom_distill...
components/dom_distiller/core/html/dom_distiller_viewer.html:12: $1
nit: could we get comments by these non-obvious placeholders?

[mdjones, 2015-05-22 00:44:48]

https://codereview.chromium.org/1130703003/diff/140001/chrome/browser/dom_dis...
File chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc
(right):

https://codereview.chromium.org/1130703003/diff/140001/chrome/browser/dom_dis...
chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc:273: //
Setup observer to inspect the RenderViewHost after committed navigation.
On 2015/05/21 20:53:44, cjhopman wrote:
> This comment doesn't make sense to me we aren't setting up an observer here.
> 
> Also, why not get this just before it's actually used?

Done.

https://codereview.chromium.org/1130703003/diff/140001/components/dom_distill...
File components/dom_distiller/core/dom_distiller_request_view_base.cc (right):

https://codereview.chromium.org/1130703003/diff/140001/components/dom_distill...
components/dom_distiller/core/dom_distiller_request_view_base.cc:55: std::string
title(article_proto->title());
On 2015/05/21 20:53:45, cjhopman wrote:
> why not just inline this in the call below? we only use it that once, right?

Done.

https://codereview.chromium.org/1130703003/diff/140001/components/dom_distill...
components/dom_distiller/core/dom_distiller_request_view_base.cc:90: std::string
title(page.title());
On 2015/05/21 20:53:45, cjhopman wrote:
> same as above

Done.

https://codereview.chromium.org/1130703003/diff/140001/components/dom_distill...
File components/dom_distiller/core/html/dom_distiller_viewer.html (right):

https://codereview.chromium.org/1130703003/diff/140001/components/dom_distill...
components/dom_distiller/core/html/dom_distiller_viewer.html:11: <title></title>
On 2015/05/21 20:53:45, cjhopman wrote:
> Should we have a title of some sort while loading?

I added a string that just contains "Reader Mode" for this title. This could be
dynamically put here but I don't think it needs to be. I don't thing we should
have anything in the "titleHolder" below.

https://codereview.chromium.org/1130703003/diff/140001/components/dom_distill...
components/dom_distiller/core/html/dom_distiller_viewer.html:12: $1
On 2015/05/21 20:53:45, cjhopman wrote:
> nit: could we get comments by these non-obvious placeholders?

Done.

[cjhopman, 2015-05-26 21:48:15]

lgtm

https://codereview.chromium.org/1130703003/diff/160001/components/dom_distill...
File components/dom_distiller/core/html/dom_distiller_viewer.html (right):

https://codereview.chromium.org/1130703003/diff/160001/components/dom_distill...
components/dom_distiller/core/html/dom_distiller_viewer.html:11:
<title>$6</title>
I'd renumber these so that they're in order... but it's not really important

[mdjones, 2015-05-27 01:01:50]

https://codereview.chromium.org/1130703003/diff/160001/components/dom_distill...
File components/dom_distiller/core/html/dom_distiller_viewer.html (right):

https://codereview.chromium.org/1130703003/diff/160001/components/dom_distill...
components/dom_distiller/core/html/dom_distiller_viewer.html:11:
<title>$6</title>
On 2015/05/26 21:48:15, cjhopman wrote:
> I'd renumber these so that they're in order... but it's not really important

Done.

[mdjones, 2015-05-27 15:36:25]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-05-27 15:36:26]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org,
cjhopman@chromium.org
Link to the patchset: https://codereview.chromium.org/1130703003/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-05-27 15:36:36]

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

[commit-bot: I haz the power, 2015-05-27 15:46:41]

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

[commit-bot: I haz the power, 2015-05-27 15:46:42]

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

[mdjones, 2015-05-27 15:56:39]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-05-27 15:56:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjhopman@chromium.org, noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/1130703003/#ps240001
(title: "Whitelist string for iOS")

[commit-bot: I haz the power, 2015-05-27 15:56:53]

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

[commit-bot: I haz the power, 2015-05-27 16:10:27]

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

[commit-bot: I haz the power, 2015-05-27 16:10:28]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mdjones, 2015-05-27 16:56:41]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-05-27 16:56:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjhopman@chromium.org, noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/1130703003/#ps260001
(title: "Add missing deps to gn")

[commit-bot: I haz the power, 2015-05-27 16:57:06]

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

[commit-bot: I haz the power, 2015-05-27 18:06:34]

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

[commit-bot: I haz the power, 2015-05-27 18:06:36]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cjhopman, 2015-05-27 22:18:09]

lgtm

[mdjones, 2015-05-27 22:33:25]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-05-27 22:33:26]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/1130703003/#ps300001
(title: "Better default direction")

[commit-bot: I haz the power, 2015-05-27 22:34:01]

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

[commit-bot: I haz the power, 2015-05-27 23:59:03]

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

[commit-bot: I haz the power, 2015-05-27 23:59:05]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mdjones, 2015-05-28 20:10:51]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-05-28 20:10:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjhopman@chromium.org, noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/1130703003/#ps320001
(title: "Fix flaky test")

[commit-bot: I haz the power, 2015-05-28 20:11:57]

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

[commit-bot: I haz the power, 2015-05-28 21:06:33]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-05-28 21:07:22]

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