Report empty article as empty.

Report empty article as empty.

Dom Distiller adds "No data found" to the HTML if the page is empty.
This prevents RL to handle correctly the error.

BUG=648923
Committed: https://crrev.com/4af6f965f87724fcfc7b7e2b900522edf57a5212
Cr-Commit-Position: refs/heads/master@{#440391}

[Olivier, 2016-12-21 13:05:18]

olivierrobin@chromium.org changed reviewers:
+ jif@chromium.org, wychen@chromium.org

[jif, 2016-12-21 13:36:33]

Ideally DistilledArticleProto would contain a boolean informing whether an error
occured or not.

https://codereview.chromium.org/2594663005/diff/1/ios/chrome/browser/dom_dist...
File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right):

https://codereview.chromium.org/2594663005/diff/1/ios/chrome/browser/dom_dist...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:44: is_empty = is_empty &&
article_proto->pages(i).html() == "";
Code that breaks out of the loop is easier to understand:

bool is_empty = true;
for (...
if (article_proto->pages(i).html() != "") {
  is_empty = false;
  break;
}
}

[Olivier, 2016-12-21 13:46:27]

True.
Wychen, can we add this flag in the proto?

https://codereview.chromium.org/2594663005/diff/1/ios/chrome/browser/dom_dist...
File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right):

https://codereview.chromium.org/2594663005/diff/1/ios/chrome/browser/dom_dist...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:44: is_empty = is_empty &&
article_proto->pages(i).html() == "";
On 2016/12/21 13:36:33, jif wrote:
> Code that breaks out of the loop is easier to understand:
> 
> bool is_empty = true;
> for (...
> if (article_proto->pages(i).html() != "") {
>   is_empty = false;
>   break;
> }
> }

Done.

[wychen, 2016-12-22 07:58:19]

I haven't built this, but with this CL, when the distilled content is an empty
string, the users would no longer see "No data found." on Clank, right?

What kind of error handling do you need on RL?

https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right):

https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:41:
DomDistillerRequestViewBase::OnArticleReady(article_proto);
In DomDistillerRequestViewBase::OnArticleReady(), "No data found" is added to
js_buffer_.

[Olivier, 2016-12-22 08:17:34]

On 2016/12/22 07:58:19, wychen wrote:
> I haven't built this, but with this CL, when the distilled content is an empty
> string, the users would no longer see "No data found." on Clank, right?
> 
> What kind of error handling do you need on RL?
> 
>
https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
> File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right):
> 
>
https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
> ios/chrome/browser/dom_distiller/distiller_viewer.cc:41:
> DomDistillerRequestViewBase::OnArticleReady(article_proto);
> In DomDistillerRequestViewBase::OnArticleReady(), "No data found" is added to
> js_buffer_.

This is in iOS directory.
In our feature, we only need a distilled version if it succeeded.
We prefer to tell the user that the page could not be distilled than showing an
empty page.

[wychen, 2016-12-22 10:19:33]

Ah. Stupid me. Didn't notice it's in ios/. :p

https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right):

https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:43: for (int i = 0; i <
article_proto->pages_size(); i++) {
When the first distilled page is empty, even if the next page link is
discovered, the link would be disregarded. The logic is here:

https://cs.chromium.org/chromium/src/components/dom_distiller/core/distiller....

We did this because the distillability heuristic could still be wrong, and if
the page is not an article, stitching a paginated part would be moot.

Given this, we only need to check the first page here.

Your current implementation is defensive, and I'm fine with it.

https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:44: if
(article_proto->pages(i).html() != "") {
Using .empty() might be slightly better.

[Olivier, 2016-12-22 10:54:59]

Thanks. PTAL

https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right):

https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:41:
DomDistillerRequestViewBase::OnArticleReady(article_proto);
On 2016/12/22 07:58:19, wychen wrote:
> In DomDistillerRequestViewBase::OnArticleReady(), "No data found" is added to
> js_buffer_.

But IIUC, not the protobuf, correct?
Should I do my check before this call?

https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:43: for (int i = 0; i <
article_proto->pages_size(); i++) {
On 2016/12/22 10:19:33, wychen wrote:
> When the first distilled page is empty, even if the next page link is
> discovered, the link would be disregarded. The logic is here:
> 
>
https://cs.chromium.org/chromium/src/components/dom_distiller/core/distiller....
> 
> We did this because the distillability heuristic could still be wrong, and if
> the page is not an article, stitching a paginated part would be moot.
> 
> Given this, we only need to check the first page here.
> 
> Your current implementation is defensive, and I'm fine with it.

Done. This makes code simpler

https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:44: if
(article_proto->pages(i).html() != "") {
On 2016/12/22 10:19:33, wychen wrote:
> Using .empty() might be slightly better.

Done.

[wychen, 2016-12-22 11:05:12]

lgtm

https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
File ios/chrome/browser/dom_distiller/distiller_viewer.cc (right):

https://codereview.chromium.org/2594663005/diff/20001/ios/chrome/browser/dom_...
ios/chrome/browser/dom_distiller/distiller_viewer.cc:41:
DomDistillerRequestViewBase::OnArticleReady(article_proto);
On 2016/12/22 10:54:59, Olivier Robin wrote:
> On 2016/12/22 07:58:19, wychen wrote:
> > In DomDistillerRequestViewBase::OnArticleReady(), "No data found" is added
to
> > js_buffer_.
> 
> But IIUC, not the protobuf, correct?
> Should I do my check before this call?

The protobuf should be intact, so no need to change. I was thinking about
calling it conditionally, but it does other things.

[Olivier, 2016-12-22 11:06:02]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2016-12-22 11:06:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Olivier, 2016-12-22 11:06:20]

Thanks for the review

[commit-bot: I haz the power, 2016-12-22 11:11:48]

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

[commit-bot: I haz the power, 2016-12-22 11:11:49]

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

[Olivier, 2016-12-22 11:14:05]

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

[Olivier, 2016-12-22 11:14:06]

+noyau as owner

[jif, 2016-12-22 12:02:51]

lgtm

[noyau (Ping after 24h), 2016-12-22 12:25:20]

The CQ bit was checked by noyau@chromium.org

[noyau (Ping after 24h), 2016-12-22 12:25:20]

lgtm

[commit-bot: I haz the power, 2016-12-22 12:25:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 12:28:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482409520273500,
"parent_rev": "229361aee3225674e106995a0a636f4d52ac3e0d", "commit_rev":
"9f88078b63b5b061ec8cc92dc6c8da0d6e01e7f1"}

[commit-bot: I haz the power, 2016-12-22 12:29:05]

Description was changed from

==========
Report empty article as empty.

Dom Distiller adds "No data found" to the HTML if the page is empty.
This prevents RL to handle correctly the error.

BUG=648923
==========

to

==========
Report empty article as empty.

Dom Distiller adds "No data found" to the HTML if the page is empty.
This prevents RL to handle correctly the error.

BUG=648923

Review-Url: https://codereview.chromium.org/2594663005
==========

[commit-bot: I haz the power, 2016-12-22 12:29:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-22 12:30:36]

Description was changed from

==========
Report empty article as empty.

Dom Distiller adds "No data found" to the HTML if the page is empty.
This prevents RL to handle correctly the error.

BUG=648923

Review-Url: https://codereview.chromium.org/2594663005
==========

to

==========
Report empty article as empty.

Dom Distiller adds "No data found" to the HTML if the page is empty.
This prevents RL to handle correctly the error.

BUG=648923

Committed: https://crrev.com/4af6f965f87724fcfc7b7e2b900522edf57a5212
Cr-Commit-Position: refs/heads/master@{#440391}
==========

[commit-bot: I haz the power, 2016-12-22 12:30:37]

Patchset 3 (id:??) landed as
https://crrev.com/4af6f965f87724fcfc7b7e2b900522edf57a5212
Cr-Commit-Position: refs/heads/master@{#440391}