Add "AllArticles" mode to Reader Mode heuristics

Add "AllArticles" mode to Reader Mode heuristics

This is a preliminary change to enable the desired mode in the back end.

BUG=736168
Review-Url: https://codereview.chromium.org/2961533002
Cr-Commit-Position: refs/heads/master@{#484987}
Committed: https://chromium.googlesource.com/chromium/src/+/4b6112cc6c198d8b6df38b3e0a9a1aa19f16b383

[wychen, 2017-06-26 23:45:23]

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

[wychen, 2017-06-26 23:45:24]

PTAL

[mdjones, 2017-06-27 16:05:18]

lgtm

[wychen, 2017-06-27 17:25:39]

The CQ bit was checked by wychen@chromium.org

[commit-bot: I haz the power, 2017-06-27 17:25:50]

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, 2017-06-27 17:40:34]

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

[commit-bot: I haz the power, 2017-06-27 17:40:35]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[wychen, 2017-06-27 17:46:53]

The CQ bit was checked by wychen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 17:47:10]

Dry run: CQ is trying da patch.

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

[wychen, 2017-06-27 17:47:23]

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

[wychen, 2017-06-27 17:47:23]

noyau@chromium.org: Could you take a look at the ios file?
 Thank you!

[commit-bot: I haz the power, 2017-06-27 18:57:48]

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

[commit-bot: I haz the power, 2017-06-27 18:57:49]

Dry run: This issue passed the CQ dry run.

[wychen, 2017-06-28 00:37:27]

https://codereview.chromium.org/2961533002/diff/20001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reader_mode/reader_mode_checker.mm (right):

https://codereview.chromium.org/2961533002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reader_mode/reader_mode_checker.mm:150: case
dom_distiller::DistillerHeuristicsType::ADABOOST_MODEL:
AFAIK, ADABOOST_MODEL doesn't reject mobile-friendly sites, right? So
ADABOOST_MODEL and ALL_ARTICLES are the same on iOS.

[noyau (Ping after 24h), 2017-06-28 08:44:06]

On 2017/06/28 00:37:27, wychen wrote:
>
https://codereview.chromium.org/2961533002/diff/20001/ios/chrome/browser/ui/r...
> File ios/chrome/browser/ui/reader_mode/reader_mode_checker.mm (right):
> 
>
https://codereview.chromium.org/2961533002/diff/20001/ios/chrome/browser/ui/r...
> ios/chrome/browser/ui/reader_mode/reader_mode_checker.mm:150: case
> dom_distiller::DistillerHeuristicsType::ADABOOST_MODEL:
> AFAIK, ADABOOST_MODEL doesn't reject mobile-friendly sites, right? So
> ADABOOST_MODEL and ALL_ARTICLES are the same on iOS.

Should the heuristic used and the mobility of the page be two switches instead
of being collated into one? And make that switch relevant on Android, but not
present on iOS?

It's weird to introduce a new heuristic, just to add a filter to an existing
one.

(And there is code in iOS allowing a user in canary and dev to change the
heuristics, if this CL land like this, this should probably be adapted as well)

[wychen, 2017-06-28 15:54:54]

On 2017/06/28 08:44:06, noyau (Ping after 24h) wrote:
> On 2017/06/28 00:37:27, wychen wrote:
> >
>
https://codereview.chromium.org/2961533002/diff/20001/ios/chrome/browser/ui/r...
> > File ios/chrome/browser/ui/reader_mode/reader_mode_checker.mm (right):
> > 
> >
>
https://codereview.chromium.org/2961533002/diff/20001/ios/chrome/browser/ui/r...
> > ios/chrome/browser/ui/reader_mode/reader_mode_checker.mm:150: case
> > dom_distiller::DistillerHeuristicsType::ADABOOST_MODEL:
> > AFAIK, ADABOOST_MODEL doesn't reject mobile-friendly sites, right? So
> > ADABOOST_MODEL and ALL_ARTICLES are the same on iOS.
> 
> Should the heuristic used and the mobility of the page be two switches instead
> of being collated into one? And make that switch relevant on Android, but not
> present on iOS?

I thought about using another switch to control the mobile-friendly filter, but
this would lead to either more implementation work or less intuitive
combinations. For example, ALWAYS with mobile-friendly filter on didn't exist.

ADABOOST_MODEL is the only one caring about mobile-friendliness anyway, so I'm
adding a variation to it.

> It's weird to introduce a new heuristic, just to add a filter to an existing
> one.
> 
> (And there is code in iOS allowing a user in canary and dev to change the
> heuristics, if this CL land like this, this should probably be adapted as
well)

How about making ALL_ARTICLES not present on iOS? This way, this CL is noop on
iOS.

[noyau (Ping after 24h), 2017-06-29 10:41:55]

On 2017/06/28 15:54:54, wychen wrote:
> On 2017/06/28 08:44:06, noyau (Ping after 24h) wrote:
> > On 2017/06/28 00:37:27, wychen wrote:
> > >
> >
>
https://codereview.chromium.org/2961533002/diff/20001/ios/chrome/browser/ui/r...
> > > File ios/chrome/browser/ui/reader_mode/reader_mode_checker.mm (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2961533002/diff/20001/ios/chrome/browser/ui/r...
> > > ios/chrome/browser/ui/reader_mode/reader_mode_checker.mm:150: case
> > > dom_distiller::DistillerHeuristicsType::ADABOOST_MODEL:
> > > AFAIK, ADABOOST_MODEL doesn't reject mobile-friendly sites, right? So
> > > ADABOOST_MODEL and ALL_ARTICLES are the same on iOS.
> > 
> > Should the heuristic used and the mobility of the page be two switches
instead
> > of being collated into one? And make that switch relevant on Android, but
not
> > present on iOS?
> 
> I thought about using another switch to control the mobile-friendly filter,
but
> this would lead to either more implementation work or less intuitive
> combinations. For example, ALWAYS with mobile-friendly filter on didn't exist.
> 
That may actually be a good thing, no? ALWAYS with a filter actually make sense…

> ADABOOST_MODEL is the only one caring about mobile-friendliness anyway, so I'm
> adding a variation to it.
> 
> > It's weird to introduce a new heuristic, just to add a filter to an existing
> > one.
> > 
> > (And there is code in iOS allowing a user in canary and dev to change the
> > heuristics, if this CL land like this, this should probably be adapted as
> well)
> 
> How about making ALL_ARTICLES not present on iOS? This way, this CL is noop on
> iOS.

Using a #define is not a good idea, it will need to be #defined everywhere. I
prefer your original solution, making ALL_ARTICLES and ADABOOST_MODEL the same
on iOS.

[wychen, 2017-07-06 18:19:18]

On 2017/06/29 10:41:55, noyau (Ping after 24h) wrote:
> On 2017/06/28 15:54:54, wychen wrote:
> > On 2017/06/28 08:44:06, noyau (Ping after 24h) wrote:
> > > On 2017/06/28 00:37:27, wychen wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2961533002/diff/20001/ios/chrome/browser/ui/r...
> > > > File ios/chrome/browser/ui/reader_mode/reader_mode_checker.mm (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2961533002/diff/20001/ios/chrome/browser/ui/r...
> > > > ios/chrome/browser/ui/reader_mode/reader_mode_checker.mm:150: case
> > > > dom_distiller::DistillerHeuristicsType::ADABOOST_MODEL:
> > > > AFAIK, ADABOOST_MODEL doesn't reject mobile-friendly sites, right? So
> > > > ADABOOST_MODEL and ALL_ARTICLES are the same on iOS.
> > > 
> > > Should the heuristic used and the mobility of the page be two switches
> instead
> > > of being collated into one? And make that switch relevant on Android, but
> not
> > > present on iOS?
> > 
> > I thought about using another switch to control the mobile-friendly filter,
> but
> > this would lead to either more implementation work or less intuitive
> > combinations. For example, ALWAYS with mobile-friendly filter on didn't
exist.
> > 
> That may actually be a good thing, no? ALWAYS with a filter actually make
sense…
> 
> > ADABOOST_MODEL is the only one caring about mobile-friendliness anyway, so
I'm
> > adding a variation to it.
> > 
> > > It's weird to introduce a new heuristic, just to add a filter to an
existing
> > > one.
> > > 
> > > (And there is code in iOS allowing a user in canary and dev to change the
> > > heuristics, if this CL land like this, this should probably be adapted as
> > well)
> > 
> > How about making ALL_ARTICLES not present on iOS? This way, this CL is noop
on
> > iOS.
> 
> Using a #define is not a good idea, it will need to be #defined everywhere. I
> prefer your original solution, making ALL_ARTICLES and ADABOOST_MODEL the same
> on iOS.

Done

[noyau (Ping after 24h), 2017-07-07 14:43:40]

lgtm

[wychen, 2017-07-07 17:14:37]

The CQ bit was checked by wychen@chromium.org

[wychen, 2017-07-07 17:14:37]

The patchset sent to the CQ was uploaded after l-g-t-m from mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2961533002/#ps60001
(title: "Revert "exclude ios"")

[commit-bot: I haz the power, 2017-07-07 17:14:47]

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, 2017-07-07 18:15:15]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499447677240600,
"parent_rev": "c4d9ba2e2790ecb45344f3b3967b0d5e3af5b5c4", "commit_rev":
"0729dbdf5e4659c9fbaca430e5aaed9d73457c86"}

[commit-bot: I haz the power, 2017-07-07 18:15:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499447677240600,
"parent_rev": "5fff83c91ced9042abc19ff2cbd41a9e9e4282e3", "commit_rev":
"4b6112cc6c198d8b6df38b3e0a9a1aa19f16b383"}

[commit-bot: I haz the power, 2017-07-07 18:15:55]

Description was changed from

==========
Add "AllArticles" mode to Reader Mode heuristics

This is a preliminary change to enable the desired mode in the back end.

BUG=736168
==========

to

==========
Add "AllArticles" mode to Reader Mode heuristics

This is a preliminary change to enable the desired mode in the back end.

BUG=736168

Review-Url: https://codereview.chromium.org/2961533002
Cr-Commit-Position: refs/heads/master@{#484987}
Committed:
https://chromium.googlesource.com/chromium/src/+/4b6112cc6c198d8b6df38b3e0a9a...
==========

[commit-bot: I haz the power, 2017-07-07 18:15:57]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4b6112cc6c198d8b6df38b3e0a9a...