Hook up new distillability signal to DistillablePageUtils

Hook up new distillability signal to DistillablePageUtils

A new push-based mechanism of distillability test (http://crrev.com/1434433002/)
is available in DistillablePageUtils.

BUG=509869
TEST=browser_tests --gtest_filter="Distillable*"

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

[wychen, 2015-11-04 09:26:35]

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

[wychen, 2015-11-04 09:26:35]

PTAL

[wychen, 2015-11-04 09:46:44]

Patchset #1 (id:1) has been deleted

[wychen, 2015-11-04 10:16:29]

Patchset #1 (id:20001) has been deleted

[mdjones, 2015-11-04 19:18:51]

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

[commit-bot: I haz the power, 2015-11-04 19:21:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414283006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414283006/40001

[commit-bot: I haz the power, 2015-11-04 19:42:33]

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

[commit-bot: I haz the power, 2015-11-04 19:42:34]

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

[mdjones, 2015-11-05 01:09:12]

https://codereview.chromium.org/1414283006/diff/60001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1414283006/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:529: new
dom_distiller::DistillabilityAgent(render_frame);
Nit: Add a comment describing why this is needed.

https://codereview.chromium.org/1414283006/diff/60001/components/dom_distille...
File components/dom_distiller/content/browser/distillable_page_utils_android.cc
(right):

https://codereview.chromium.org/1414283006/diff/60001/components/dom_distille...
components/dom_distiller/content/browser/distillable_page_utils_android.cc:68:
ScopedJavaGlobalRef<jobject>* callback_holder(
Try extending jobject to log when it is destructed.

[wychen, 2015-11-05 02:25:33]

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

[wychen, 2015-11-05 02:25:33]

+thestig@: Could you take a look at
chrome/renderer/chrome_content_renderer_client.cc? Thanks!

[wychen, 2015-11-11 21:35:31]

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

[wychen, 2015-11-11 21:35:32]

Tommy, could you take a look?

[Lei Zhang, 2015-11-11 23:12:46]

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right):

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
2015 is almost over

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:77:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
How well will the timing work if the bot is being slow?

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:94: for
(unsigned i = 0; i < 2; i++) {
just use int, ++i

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:136: typedef
DistillablePageUtilsBrowserTestOption<kAdaBoost>
using foo = bar, instead of typedef bar foo

https://codereview.chromium.org/1414283006/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1414283006/diff/60001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:108:
'browser/dom_distiller/distillable_page_utils_browsertest.cc',
alphabetical order

[wychen, 2015-11-12 05:02:58]

Description was changed from

==========
Hook up new distillability signal to DistillablePageUtils

BUG=509869
TEST=browser_tests --gtest_filter="Distillable*"
==========

to

==========
Hook up new distillability signal to DistillablePageUtils

A new push-based mechanism of distillability test (http://crrev.com/1434433002/)
is available in DistillablePageUtils.

BUG=509869
TEST=browser_tests --gtest_filter="Distillable*"
==========

[wychen, 2015-11-12 05:39:28]

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right):

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/11/11 23:12:46, Lei Zhang wrote:
> 2015 is almost over

Good catch!

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:77:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2015/11/11 23:12:46, Lei Zhang wrote:
> How well will the timing work if the bot is being slow?

I tried waiting for only 1ms, and the error rate is 2/100 locally. Waiting for
100ms should be pretty safe.

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:94: for
(unsigned i = 0; i < 2; i++) {
On 2015/11/11 23:12:46, Lei Zhang wrote:
> just use int, ++i

Done.

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:136: typedef
DistillablePageUtilsBrowserTestOption<kAdaBoost>
On 2015/11/11 23:12:46, Lei Zhang wrote:
> using foo = bar, instead of typedef bar foo

Thanks for this C++11 trick!

https://codereview.chromium.org/1414283006/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1414283006/diff/60001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:108:
'browser/dom_distiller/distillable_page_utils_browsertest.cc',
On 2015/11/11 23:12:46, Lei Zhang wrote:
> alphabetical order

Done.

https://codereview.chromium.org/1414283006/diff/60001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1414283006/diff/60001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:529: new
dom_distiller::DistillabilityAgent(render_frame);
On 2015/11/05 01:09:12, mdjones wrote:
> Nit: Add a comment describing why this is needed.

Done.

https://codereview.chromium.org/1414283006/diff/60001/components/dom_distille...
File components/dom_distiller/content/browser/distillable_page_utils_android.cc
(right):

https://codereview.chromium.org/1414283006/diff/60001/components/dom_distille...
components/dom_distiller/content/browser/distillable_page_utils_android.cc:68:
ScopedJavaGlobalRef<jobject>* callback_holder(
On 2015/11/05 01:09:12, mdjones wrote:
> Try extending jobject to log when it is destructed.

Turned out to be more difficult than I thought. Skip this test.

[Lei Zhang, 2015-11-12 07:08:19]

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right):

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:77:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2015/11/12 05:39:28, wychen wrote:
> On 2015/11/11 23:12:46, Lei Zhang wrote:
> > How well will the timing work if the bot is being slow?
> 
> I tried waiting for only 1ms, and the error rate is 2/100 locally. Waiting for
> 100ms should be pretty safe.

Is there some other signal you can listen for? Memory tools incur slowdowns and
what may seem like a long wait on normal bots can actually time out on some
memory bots.

https://codereview.chromium.org/1414283006/diff/80001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right):

https://codereview.chromium.org/1414283006/diff/80001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:28: const
char* kSimpleArticlePath = "/dom_distiller/simple_article.html";
const char foo[] = "bar";

with const char* qux = baz, you can still accidentally change what qux is
pointing to.

[wychen, 2015-11-12 22:42:52]

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right):

https://codereview.chromium.org/1414283006/diff/60001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:77:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2015/11/12 07:08:19, Lei Zhang wrote:
> On 2015/11/12 05:39:28, wychen wrote:
> > On 2015/11/11 23:12:46, Lei Zhang wrote:
> > > How well will the timing work if the bot is being slow?
> > 
> > I tried waiting for only 1ms, and the error rate is 2/100 locally. Waiting
for
> > 100ms should be pretty safe.
> 
> Is there some other signal you can listen for? Memory tools incur slowdowns
and
> what may seem like a long wait on normal bots can actually time out on some
> memory bots.

Unfortunately we couldn't guarantee the delegate would be called before a known
signal. The only thing we know is that the delegate would be called slightly
after the first paint after onload(). If 100X safety margin turns out to be not
enough for memory bots, we can increase the wait even longer later on.

One possible alternative is to create a run_loop and terminate it in the
MockDelegate. However, we can no longer use EXPECT_CALL, and the test code would
be much longer.

https://codereview.chromium.org/1414283006/diff/80001/chrome/browser/dom_dist...
File chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc (right):

https://codereview.chromium.org/1414283006/diff/80001/chrome/browser/dom_dist...
chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc:28: const
char* kSimpleArticlePath = "/dom_distiller/simple_article.html";
On 2015/11/12 07:08:19, Lei Zhang wrote:
> const char foo[] = "bar";
> 
> with const char* qux = baz, you can still accidentally change what qux is
> pointing to.

Done.

[Lei Zhang, 2015-11-12 22:49:34]

On 2015/11/12 22:42:52, wychen wrote:
> Unfortunately we couldn't guarantee the delegate would be called before a
known
> signal. The only thing we know is that the delegate would be called slightly
> after the first paint after onload(). If 100X safety margin turns out to be
not
> enough for memory bots, we can increase the wait even longer later on.

I don't know enough about the distiller code to know how much work it would be
to redesign it with an observable event so you don't have to wait in this
manner. It would be nice to do that later if possible. We can see if the 100X
safety margin is sufficient and how much these tests actually flake.

> One possible alternative is to create a run_loop and terminate it in the
> MockDelegate. However, we can no longer use EXPECT_CALL, and the test code
would
> be much longer.

Ack. And I would not want your tests to become more complicated.

[Lei Zhang, 2015-11-12 22:51:37]

https://codereview.chromium.org/1414283006/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1414283006/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:1584:
switches::kReaderModeHeuristics,
You are forwarding this switch to renderers, but it's not obvious where in the
renderer it's actually being used. Can you point that out?

[wychen, 2015-11-12 23:07:03]

https://codereview.chromium.org/1414283006/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1414283006/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:1584:
switches::kReaderModeHeuristics,
On 2015/11/12 22:51:37, Lei Zhang wrote:
> You are forwarding this switch to renderers, but it's not obvious where in the
> renderer it's actually being used. Can you point that out?

I guess this conceptually belongs to the CL this depends on.
I've moved it there https://codereview.chromium.org/1434433002/.

[Lei Zhang, 2015-11-12 23:11:18]

chrome/ lgtm

[mdjones, 2015-11-12 23:46:48]

ltgm with nit

https://codereview.chromium.org/1414283006/diff/120001/components/dom_distill...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/content/DistillablePageUtils.java
(right):

https://codereview.chromium.org/1414283006/diff/120001/components/dom_distill...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/content/DistillablePageUtils.java:41:
public void onIsPageDistillableResult(boolean isDistillable, boolean isLast);
Nit: javadoc for public interface.

[wychen, 2015-11-13 01:55:12]

Matt, you wrote ltgm, not lgtm.

https://codereview.chromium.org/1414283006/diff/120001/components/dom_distill...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/content/DistillablePageUtils.java
(right):

https://codereview.chromium.org/1414283006/diff/120001/components/dom_distill...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/content/DistillablePageUtils.java:41:
public void onIsPageDistillableResult(boolean isDistillable, boolean isLast);
On 2015/11/12 23:46:48, mdjones wrote:
> Nit: javadoc for public interface.

Done.

[mdjones, 2015-11-13 03:30:10]

On 2015/11/13 01:55:12, wychen wrote:
> Matt, you wrote ltgm, not lgtm.
> 
>
https://codereview.chromium.org/1414283006/diff/120001/components/dom_distill...
> File
>
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/content/DistillablePageUtils.java
> (right):
> 
>
https://codereview.chromium.org/1414283006/diff/120001/components/dom_distill...
>
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/content/DistillablePageUtils.java:41:
> public void onIsPageDistillableResult(boolean isDistillable, boolean isLast);
> On 2015/11/12 23:46:48, mdjones wrote:
> > Nit: javadoc for public interface.
> 
> Done.

lgtm

[wychen, 2015-11-13 09:21:18]

Patchset #7 (id:160001) has been deleted

[wychen, 2015-11-13 09:21:49]

Patchset #7 (id:180001) has been deleted

[wychen, 2015-11-13 09:22:41]

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

[commit-bot: I haz the power, 2015-11-13 09:23:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414283006/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414283006/200001

[commit-bot: I haz the power, 2015-11-13 09:30:56]

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

[commit-bot: I haz the power, 2015-11-13 09:30:57]

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

[wychen, 2015-11-13 20:19:05]

Patchset #8 (id:220001) has been deleted

[wychen, 2015-11-13 20:19:30]

The CQ bit was checked by wychen@chromium.org

[wychen, 2015-11-13 20:19:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/1414283006/#ps200001
(title: "merge depend")

[commit-bot: I haz the power, 2015-11-13 20:21:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414283006/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414283006/200001

[commit-bot: I haz the power, 2015-11-13 21:01:14]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2015-11-13 21:02:03]

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