Pass distillability updates from renderer to browser

Pass distillability updates from renderer to browser

The current distillability test does the following:
- In WebContentsObserver, wait for didFinishLoad and didFailLoad events.
  (Was didFinishLoad and documentLoadedInFrame earlier. Read more on
   http://crbug.com/510254.)
- In these events, send feature extraction JavaScript code to renderer.

The problems are:
- Mobile-friendliness detection might not be accurate because layout might
  not have been done in these WebContentsObserver events.
- Executing JavaScript in an isolated world costs extra 8MB of memory.

With the new mechanism added in this CL, the problems can be solved.
- DidMeaningfulLayout guarantees layout is done, so mobile-friendliness
  hint would be accurate.
- Features are extracted in native code (in http://crrev.com/1419033004/).

The reader mode switch kReaderModeHeuristics is passed to the renderer
process for it to use GetDistillerHeuristicsType().

BUG=509869, 525797
Committed: https://crrev.com/e556873497657ad7ce407109fa3a406fad6f61bf
Cr-Commit-Position: refs/heads/master@{#359607}

[wychen, 2015-11-04 09:25:08]

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

[wychen, 2015-11-04 09:25:08]

PTAL

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

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:18:53]

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

[commit-bot: I haz the power, 2015-11-04 19:44:24]

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

[commit-bot: I haz the power, 2015-11-04 19:44:25]

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-05 00:38:01]

Patchset #2 (id:20001) has been deleted

[wychen, 2015-11-05 02:22:23]

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

[wychen, 2015-11-05 02:22:23]

+palmer@. Could you take a look at the IPC file? Thanks!

[blundell, 2015-11-05 09:42:41]

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

[blundell, 2015-11-05 09:42:41]

https://codereview.chromium.org/1434433002/diff/60001/components/dom_distille...
File components/dom_distiller/content/browser/distillability_driver.h (right):

https://codereview.chromium.org/1434433002/diff/60001/components/dom_distille...
components/dom_distiller/content/browser/distillability_driver.h:15: class
WebContentsDistillabilityDriver
drive-by nit: this should be named web_contents_dom_distillability_driver.

[mdjones, 2015-11-10 16:53:42]

lgtm with name issue resolved.

[palmer, 2015-11-10 19:49:48]

lgtm

[wychen, 2015-11-11 21:34:42]

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

[wychen, 2015-11-11 21:34:43]

Tommy, could you also take a look? Thanks!

[nyquist, 2015-11-11 21:53:45]

lgtm

https://codereview.chromium.org/1434433002/diff/60001/components/dom_distille...
File components/dom_distiller/content/renderer/distillability_agent.cc (right):

https://codereview.chromium.org/1434433002/diff/60001/components/dom_distille...
components/dom_distiller/content/renderer/distillability_agent.cc:31: bool
needToUpdate(bool is_loaded) {
Nit: NeedToUpdate (and IsLast below)

https://codereview.chromium.org/1434433002/diff/60001/components/dom_distille...
components/dom_distiller/content/renderer/distillability_agent.cc:53: return
is_loaded;
Optional nit:

Could this just be:
###
if (GetDistillerHeuristicsType() == DistillerHeuristicsType::ADABOOST_MODEL)
  return is_loaded;

return true;
###

?
I don't feel strongly though.

[nyquist, 2015-11-11 22:24:26]

Also; could you expand the CL description?

Maybe try to answer these questions:
- How did updates go from the renderer to the browser before?
- Why is that bad?
- How / why does this make it better?

[wychen, 2015-11-12 02:00:40]

Description was changed from

==========
Pass distillability updates from renderer to browser

BUG=509869
==========

to

==========
Pass distillability updates from renderer to browser

The current distillability test is done this way:
- In WebContentsObserver, wait for didFinishLoad and didFailLoad events.
  (Was didFinishLoad and documentLoadedInFrame earlier. Read more on
   http://crbug.com/510254.)
- In these events, send feature extraction JavaScript code to renderer.

The problems are:
- Mobile-friendliness detection might not be accurate because layout might
  not have been done in these WebContentsObserver events.
- Executing JavaScript in an isolated world costs extra 8MB of memory.

With the new mechanism added in this CL, the problems can be solved.
- DidMeaningfulLayout guarantees layout is done, so mobile-friendliness
  hint would be accurate.
- Features are extracted in native code (in http://crrev.com/1419033004/).

BUG=509869, 525797
==========

[wychen, 2015-11-12 04:59:45]

https://codereview.chromium.org/1434433002/diff/60001/components/dom_distille...
File components/dom_distiller/content/browser/distillability_driver.h (right):

https://codereview.chromium.org/1434433002/diff/60001/components/dom_distille...
components/dom_distiller/content/browser/distillability_driver.h:15: class
WebContentsDistillabilityDriver
On 2015/11/05 09:42:41, blundell wrote:
> drive-by nit: this should be named web_contents_dom_distillability_driver.

Changed the class name.

https://codereview.chromium.org/1434433002/diff/60001/components/dom_distille...
File components/dom_distiller/content/renderer/distillability_agent.cc (right):

https://codereview.chromium.org/1434433002/diff/60001/components/dom_distille...
components/dom_distiller/content/renderer/distillability_agent.cc:31: bool
needToUpdate(bool is_loaded) {
On 2015/11/11 21:53:45, nyquist wrote:
> Nit: NeedToUpdate (and IsLast below)

Done.

https://codereview.chromium.org/1434433002/diff/60001/components/dom_distille...
components/dom_distiller/content/renderer/distillability_agent.cc:53: return
is_loaded;
On 2015/11/11 21:53:45, nyquist wrote:
> Optional nit:
> 
> Could this just be:
> ###
> if (GetDistillerHeuristicsType() == DistillerHeuristicsType::ADABOOST_MODEL)
>   return is_loaded;
> 
> return true;
> ###
> 
> ?
> I don't feel strongly though.

Done.

[wychen, 2015-11-12 23:01:06]

Description was changed from

==========
Pass distillability updates from renderer to browser

The current distillability test is done this way:
- In WebContentsObserver, wait for didFinishLoad and didFailLoad events.
  (Was didFinishLoad and documentLoadedInFrame earlier. Read more on
   http://crbug.com/510254.)
- In these events, send feature extraction JavaScript code to renderer.

The problems are:
- Mobile-friendliness detection might not be accurate because layout might
  not have been done in these WebContentsObserver events.
- Executing JavaScript in an isolated world costs extra 8MB of memory.

With the new mechanism added in this CL, the problems can be solved.
- DidMeaningfulLayout guarantees layout is done, so mobile-friendliness
  hint would be accurate.
- Features are extracted in native code (in http://crrev.com/1419033004/).

BUG=509869, 525797
==========

to

==========
Pass distillability updates from renderer to browser

The current distillability test is done this way:
- In WebContentsObserver, wait for didFinishLoad and didFailLoad events.
  (Was didFinishLoad and documentLoadedInFrame earlier. Read more on
   http://crbug.com/510254.)
- In these events, send feature extraction JavaScript code to renderer.

The problems are:
- Mobile-friendliness detection might not be accurate because layout might
  not have been done in these WebContentsObserver events.
- Executing JavaScript in an isolated world costs extra 8MB of memory.

With the new mechanism added in this CL, the problems can be solved.
- DidMeaningfulLayout guarantees layout is done, so mobile-friendliness
  hint would be accurate.
- Features are extracted in native code (in http://crrev.com/1419033004/).

The reader mode switch kReaderModeHeuristics is passed to the render process
for it to use GetDistillerHeuristicsType().

BUG=509869, 525797
==========

[Lei Zhang, 2015-11-12 23:10:50]

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

[Lei Zhang, 2015-11-12 23:10:50]

chrome/ lgtm

nit: In the CL desc, "render process" -> "renderer process"

[wychen, 2015-11-12 23:20:08]

Description was changed from

==========
Pass distillability updates from renderer to browser

The current distillability test is done this way:
- In WebContentsObserver, wait for didFinishLoad and didFailLoad events.
  (Was didFinishLoad and documentLoadedInFrame earlier. Read more on
   http://crbug.com/510254.)
- In these events, send feature extraction JavaScript code to renderer.

The problems are:
- Mobile-friendliness detection might not be accurate because layout might
  not have been done in these WebContentsObserver events.
- Executing JavaScript in an isolated world costs extra 8MB of memory.

With the new mechanism added in this CL, the problems can be solved.
- DidMeaningfulLayout guarantees layout is done, so mobile-friendliness
  hint would be accurate.
- Features are extracted in native code (in http://crrev.com/1419033004/).

The reader mode switch kReaderModeHeuristics is passed to the render process
for it to use GetDistillerHeuristicsType().

BUG=509869, 525797
==========

to

==========
Pass distillability updates from renderer to browser

The current distillability test is done this way:
- In WebContentsObserver, wait for didFinishLoad and didFailLoad events.
  (Was didFinishLoad and documentLoadedInFrame earlier. Read more on
   http://crbug.com/510254.)
- In these events, send feature extraction JavaScript code to renderer.

The problems are:
- Mobile-friendliness detection might not be accurate because layout might
  not have been done in these WebContentsObserver events.
- Executing JavaScript in an isolated world costs extra 8MB of memory.

With the new mechanism added in this CL, the problems can be solved.
- DidMeaningfulLayout guarantees layout is done, so mobile-friendliness
  hint would be accurate.
- Features are extracted in native code (in http://crrev.com/1419033004/).

The reader mode switch kReaderModeHeuristics is passed to the renderer
process for it to use GetDistillerHeuristicsType().

BUG=509869, 525797
==========

[wychen, 2015-11-12 23:20:38]

On 2015/11/12 23:10:50, Lei Zhang wrote:
> chrome/ lgtm
> 
> nit: In the CL desc, "render process" -> "renderer process"

Done.

[wychen, 2015-11-13 02:13:19]

Description was changed from

==========
Pass distillability updates from renderer to browser

The current distillability test is done this way:
- In WebContentsObserver, wait for didFinishLoad and didFailLoad events.
  (Was didFinishLoad and documentLoadedInFrame earlier. Read more on
   http://crbug.com/510254.)
- In these events, send feature extraction JavaScript code to renderer.

The problems are:
- Mobile-friendliness detection might not be accurate because layout might
  not have been done in these WebContentsObserver events.
- Executing JavaScript in an isolated world costs extra 8MB of memory.

With the new mechanism added in this CL, the problems can be solved.
- DidMeaningfulLayout guarantees layout is done, so mobile-friendliness
  hint would be accurate.
- Features are extracted in native code (in http://crrev.com/1419033004/).

The reader mode switch kReaderModeHeuristics is passed to the renderer
process for it to use GetDistillerHeuristicsType().

BUG=509869, 525797
==========

to

==========
Pass distillability updates from renderer to browser

The current distillability test does the following:
- In WebContentsObserver, wait for didFinishLoad and didFailLoad events.
  (Was didFinishLoad and documentLoadedInFrame earlier. Read more on
   http://crbug.com/510254.)
- In these events, send feature extraction JavaScript code to renderer.

The problems are:
- Mobile-friendliness detection might not be accurate because layout might
  not have been done in these WebContentsObserver events.
- Executing JavaScript in an isolated world costs extra 8MB of memory.

With the new mechanism added in this CL, the problems can be solved.
- DidMeaningfulLayout guarantees layout is done, so mobile-friendliness
  hint would be accurate.
- Features are extracted in native code (in http://crrev.com/1419033004/).

The reader mode switch kReaderModeHeuristics is passed to the renderer
process for it to use GetDistillerHeuristicsType().

BUG=509869, 525797
==========

[wychen, 2015-11-13 08:06:25]

The CQ bit was checked by wychen@chromium.org

[wychen, 2015-11-13 08:06:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, palmer@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1434433002/#ps100001
(title: "pass reader mode switches to the render process")

[commit-bot: I haz the power, 2015-11-13 08:06:53]

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

[commit-bot: I haz the power, 2015-11-13 08:08:29]

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

[commit-bot: I haz the power, 2015-11-13 08:08:29]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wychen, 2015-11-13 09:03:54]

Patchset #6 (id:120001) has been deleted

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

Patchset #6 (id:140001) has been deleted

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

The CQ bit was checked by wychen@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, palmer@chromium.org, thestig@chromium.org,
nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1434433002/#ps180001
(title: "fix merging")

[commit-bot: I haz the power, 2015-11-13 09:18:50]

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

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

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

[commit-bot: I haz the power, 2015-11-13 09:26:27]

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 09:41:55]

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:42:35]

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

[wychen, 2015-11-13 09:48:36]

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

[wychen, 2015-11-13 09:48:37]

+tsepez@, could you take a look at components/dom_distiller/content/DEPS for
adding +ipc? Thanks!

[commit-bot: I haz the power, 2015-11-13 09:50:03]

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

[commit-bot: I haz the power, 2015-11-13 09:50:03]

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...)

[Tom Sepez, 2015-11-13 16:59:41]

DEPS lgtm.

[Tom Sepez, 2015-11-13 17:00:44]

https://codereview.chromium.org/1434433002/diff/200001/components/dom_distill...
File components/dom_distiller/content/common/distiller_messages.h (right):

https://codereview.chromium.org/1434433002/diff/200001/components/dom_distill...
components/dom_distiller/content/common/distiller_messages.h:19:
IPC_MESSAGE_ROUTED2(FrameHostMsg_Distillability, bool, bool)
nit: add comment explaining what each bool means.

[wychen, 2015-11-13 18:03:27]

The CQ bit was checked by wychen@chromium.org

[wychen, 2015-11-13 18:03:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, palmer@chromium.org, thestig@chromium.org,
nyquist@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1434433002/#ps220001
(title: "address tsepez's comment")

[commit-bot: I haz the power, 2015-11-13 18:04:04]

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

[wychen, 2015-11-13 18:04:56]

https://codereview.chromium.org/1434433002/diff/200001/components/dom_distill...
File components/dom_distiller/content/common/distiller_messages.h (right):

https://codereview.chromium.org/1434433002/diff/200001/components/dom_distill...
components/dom_distiller/content/common/distiller_messages.h:19:
IPC_MESSAGE_ROUTED2(FrameHostMsg_Distillability, bool, bool)
On 2015/11/13 17:00:44, Tom Sepez wrote:
> nit: add comment explaining what each bool means.

Done.

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

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2015-11-13 20:18:26]

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