Test distillability without JavaScript

Test distillability without JavaScript

In order to save memory usage, a native version of feature extraction is used
for distillability test. The features are inspired by the original
extract_features.js code.

Some tests are moved from components_browsertests to browser_tests because
DistillabilityAgent() is created in chrome layer renderer.

BUG=509869
TEST=browser_tests --gtest_filter="Distillable*"
TEST=components_browsertests --gtest_filter="DomDistillerDistillablePageUtilsTest.*"
TEST=components_unittests --gtest_filter="DomDistillerPageFeaturesTest*"

[wychen, 2015-07-22 23:02:52]

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

[wychen, 2015-07-22 23:02:53]

PTAL.

This relies on Blink changes in https://codereview.chromium.org/1255483003/, and
contentAsTextForTesting(). These should be fixed.

It would be nice if you could comment on other things first.

[cjhopman, 2015-07-22 23:34:03]

I really don't think we should be putting so much dom_distiller implementation
into content/.

We should be able to pull up most of the ipc stuff into
components/dom_distiller/content/...

You can see how some of the other components deal with this by looking for
*_messages.h files in components/ and following how it's implemented.

Another possibility would be to use mojo instead of the normal ipc stuff. I
don't know if chrome currently really encourages one over the other or not
(though I personally prefer mojo).

[dglazkov, 2015-07-23 20:54:14]

We shouldn't be doing DOM Walking outside of Blink. Please avoid writing DOM
traversal/introspection code in Chromium-land.

[wychen, 2015-09-26 03:49:03]

wychen@chromium.org changed reviewers:
- cjhopman@chromium.org

[wychen, 2015-09-28 18:34:45]

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

[wychen, 2015-09-28 18:34:46]

mdjones: PTAL. The WebKit part is not done yet though. The DOM traversal in
distillability_agent.cc would be moved to WebKit.

[mdjones, 2015-09-28 18:46:50]

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

[commit-bot: I haz the power, 2015-09-28 18:47:22]

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

[commit-bot: I haz the power, 2015-09-28 19:13:29]

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

[commit-bot: I haz the power, 2015-09-28 19:13:30]

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-09-29 05:50:30]

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

[commit-bot: I haz the power, 2015-09-29 05:50:43]

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

[commit-bot: I haz the power, 2015-09-29 05:50:45]

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

[commit-bot: I haz the power, 2015-09-29 05:50:45]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mdjones, 2015-09-29 15:49:00]

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

[commit-bot: I haz the power, 2015-09-29 15:49:19]

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

[commit-bot: I haz the power, 2015-09-29 15:59:35]

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

[commit-bot: I haz the power, 2015-09-29 15:59:35]

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-09-29 17:31:42]

https://codereview.chromium.org/1248643004/diff/140001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/1248643004/diff/140001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:44: #include
"components/dom_distiller/content/browser/distillability_driver.h"
Is this needed?

https://codereview.chromium.org/1248643004/diff/140001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1248643004/diff/140001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:587: new
dom_distiller::DistillabilityAgent(render_frame);
Nit: It might be good to have this close to the other distiller code in this
function.

https://codereview.chromium.org/1248643004/diff/140001/components/dom_distill...
File components/dom_distiller/content/browser/distillable_page_utils.cc (right):

https://codereview.chromium.org/1248643004/diff/140001/components/dom_distill...
components/dom_distiller/content/browser/distillable_page_utils.cc:35: #if
defined(OS_IOS)
I don't think we need this for code in the dom_distiller/content directory since
content is not used in iOS.

It would be nice to have this logic somewhere in core that can be overridden
somehow. I think that would be a separate change though since I don't believe
the detector runs on iOS.

https://codereview.chromium.org/1248643004/diff/140001/components/dom_distill...
components/dom_distiller/content/browser/distillable_page_utils.cc:130: #if
defined(OS_IOS)
Same as above (not needed for content).

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

https://codereview.chromium.org/1248643004/diff/140001/components/dom_distill...
components/dom_distiller/content/common/distiller_messages.h:18:
IPC_STRUCT_BEGIN(FrameHostMsg_ExtractFeatureResponse_Params)
In the future, this would be a good candidate for mojo.

https://codereview.chromium.org/1248643004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp (right):

https://codereview.chromium.org/1248643004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:92: //
document.addConsoleMessage(consoleMessage);
Is this logging code permanent?

[wychen, 2015-09-29 21:46:57]

https://codereview.chromium.org/1248643004/diff/140001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/1248643004/diff/140001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:44: #include
"components/dom_distiller/content/browser/distillability_driver.h"
On 2015/09/29 17:31:42, mdjones wrote:
> Is this needed?

Done.

https://codereview.chromium.org/1248643004/diff/140001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1248643004/diff/140001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:587: new
dom_distiller::DistillabilityAgent(render_frame);
On 2015/09/29 17:31:42, mdjones wrote:
> Nit: It might be good to have this close to the other distiller code in this
> function.

Done.

https://codereview.chromium.org/1248643004/diff/140001/components/dom_distill...
File components/dom_distiller/content/browser/distillable_page_utils.cc (right):

https://codereview.chromium.org/1248643004/diff/140001/components/dom_distill...
components/dom_distiller/content/browser/distillable_page_utils.cc:35: #if
defined(OS_IOS)
On 2015/09/29 17:31:42, mdjones wrote:
> I don't think we need this for code in the dom_distiller/content directory
since
> content is not used in iOS.
> 
> It would be nice to have this logic somewhere in core that can be overridden
> somehow. I think that would be a separate change though since I don't believe
> the detector runs on iOS.

Done.

https://codereview.chromium.org/1248643004/diff/140001/components/dom_distill...
components/dom_distiller/content/browser/distillable_page_utils.cc:130: #if
defined(OS_IOS)
On 2015/09/29 17:31:42, mdjones wrote:
> Same as above (not needed for content).

Done.

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

https://codereview.chromium.org/1248643004/diff/140001/components/dom_distill...
components/dom_distiller/content/common/distiller_messages.h:18:
IPC_STRUCT_BEGIN(FrameHostMsg_ExtractFeatureResponse_Params)
On 2015/09/29 17:31:42, mdjones wrote:
> In the future, this would be a good candidate for mojo.

Totally agree.

https://codereview.chromium.org/1248643004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp (right):

https://codereview.chromium.org/1248643004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:92: //
document.addConsoleMessage(consoleMessage);
On 2015/09/29 17:31:42, mdjones wrote:
> Is this logging code permanent?

The WebKit part is not done yet. This is just for debugging only.

[wychen, 2015-10-21 23:26:10]

wychen@chromium.org changed reviewers:
+ dglazkov@chromium.org, esprehn@chromium.org

[wychen, 2015-10-21 23:26:11]

dglazkov, esprehn: Could you take a look at the WebKit part? The connection with
the UI and the IPC is still working in progress.

[dglazkov, 2015-10-22 16:30:31]

Made a quick pass, there's lots of work to do there.

One thing to consider -- let's break this up early into smaller patches. For
example, a good first Blink-side patch would be to simply introduce the
distillability hook that does nothing. Then, we can land small patches to add
necessary bits.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:1057: WebDistillabilityFeatures
distillabilityFeatures();
There's no need for this to be a Document member. Think of
DistillabilityFeatureExtractor thingy as something that operates on Document,
but doesn't necessarily have any other relationship with it.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:1402: DocumentStatisticsCollector
m_statisticsCollector;
Does not need to be a member.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp (right):

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:31: return
root.textContent().stripWhiteSpace().length();
The TODO needs to be addressed before landing.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:45: class
ExtractFeatureWalker {
This can just be a separate class, no need to hide it. Will likely need a
different name then.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:51:
unlikelyCandidates = {"banner", "combx", "comment", "community", "disqus",
"extra", "foot", "header", "menu", "related", "remark", "rss", "share",
"shoutbox", "sidebar", "skyscraper", "sponsor", "ad-break", "agegate",
"pagination", "pager", "popup"};
Should these be static? Why initialize them in constructor?

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:68: String
hay = elem.getClassAttribute().lower() + " " + elem.getIdAttribute().lower();
We have lots of style machinery to do this correctly. Please look over
StyleResolver and friends. Now that you're in Blink, you no longer need to treat
that machinery as opaque.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:130: for
(const Node& node : NodeTraversal::childrenOf(head)) {
If you're traversing things in the method above, might as well do this in there,
too.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.h (right):

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.h:25: bool
m_readyToCollect : 1;
I don't think you need this. Maybe just have an assert around document lifecycle
in collectStatistics?

[wychen, 2015-10-23 02:51:30]

Thanks for reviewing!

I've split the Blink-side to another CL:
https://codereview.chromium.org/1419033004/

It is the same as patch set 13 here.

Let's continue from there.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:1057: WebDistillabilityFeatures
distillabilityFeatures();
On 2015/10/22 16:30:31, dglazkov wrote:
> There's no need for this to be a Document member. Think of
> DistillabilityFeatureExtractor thingy as something that operates on Document,
> but doesn't necessarily have any other relationship with it.

Done.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:1402: DocumentStatisticsCollector
m_statisticsCollector;
On 2015/10/22 16:30:31, dglazkov wrote:
> Does not need to be a member.

Done.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp (right):

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:31: return
root.textContent().stripWhiteSpace().length();
On 2015/10/22 16:30:31, dglazkov wrote:
> The TODO needs to be addressed before landing.

Will do.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:45: class
ExtractFeatureWalker {
On 2015/10/22 16:30:31, dglazkov wrote:
> This can just be a separate class, no need to hide it. Will likely need a
> different name then.

Do you mean creating new cpp/h files for this class in WebKit/Source/core/dom/?
I don't understand why hiding it is not a good idea. This class is only used
here.

Or merging it with class DocumentStatisticsCollector?

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:51:
unlikelyCandidates = {"banner", "combx", "comment", "community", "disqus",
"extra", "foot", "header", "menu", "related", "remark", "rss", "share",
"shoutbox", "sidebar", "skyscraper", "sponsor", "ad-break", "agegate",
"pagination", "pager", "popup"};
On 2015/10/22 16:30:31, dglazkov wrote:
> Should these be static? Why initialize them in constructor?

Made static.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:68: String
hay = elem.getClassAttribute().lower() + " " + elem.getIdAttribute().lower();
On 2015/10/22 16:30:31, dglazkov wrote:
> We have lots of style machinery to do this correctly. Please look over
> StyleResolver and friends. Now that you're in Blink, you no longer need to
treat
> that machinery as opaque.

Could you elaborate how to use StyleResolver to improve this?

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:130: for
(const Node& node : NodeTraversal::childrenOf(head)) {
On 2015/10/22 16:30:31, dglazkov wrote:
> If you're traversing things in the method above, might as well do this in
there,
> too.

hasOGArticle traverses head, while walk() traverses body, so I've moved this as
a separate member function of ExtractFeatureWalker.

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.h (right):

https://codereview.chromium.org/1248643004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.h:25: bool
m_readyToCollect : 1;
On 2015/10/22 16:30:31, dglazkov wrote:
> I don't think you need this. Maybe just have an assert around document
lifecycle
> in collectStatistics?

Removed.

[wychen, 2015-10-28 07:46:48]

Patchset #14 (id:260001) has been deleted

[wychen, 2015-10-30 18:03:10]

Description was changed from

==========
Port extract_features.js to native code

In order to save memory usage, a native version of extract_features.js is
added.

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

to

==========
Port extract_features.js to native code

In order to save memory usage, a native version of extract_features.js is
added.

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

[wychen, 2015-10-30 18:06:14]

Description was changed from

==========
Port extract_features.js to native code

In order to save memory usage, a native version of extract_features.js is
added.

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

to

==========
Test distillability without JavaScript

In order to save memory usage, a native version of feature extraction is used
for distillability test. The features are inspired by the original
extract_features.js code.

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

[wychen, 2015-11-01 03:41:18]

Description was changed from

==========
Test distillability without JavaScript

In order to save memory usage, a native version of feature extraction is used
for distillability test. The features are inspired by the original
extract_features.js code.

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

to

==========
Test distillability without JavaScript

In order to save memory usage, a native version of feature extraction is used
for distillability test. The features are inspired by the original
extract_features.js code.

Some tests are moved from components_browsertests to browser_tests because
DistillabilityAgent() is created in chrome layer renderer.

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

[wychen, 2015-11-01 19:50:29]

Description was changed from

==========
Test distillability without JavaScript

In order to save memory usage, a native version of feature extraction is used
for distillability test. The features are inspired by the original
extract_features.js code.

Some tests are moved from components_browsertests to browser_tests because
DistillabilityAgent() is created in chrome layer renderer.

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

to

==========
Test distillability without JavaScript

In order to save memory usage, a native version of feature extraction is used
for distillability test. The features are inspired by the original
extract_features.js code.

Some tests are moved from components_browsertests to browser_tests because
DistillabilityAgent() is created in chrome layer renderer.

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

[wychen, 2015-11-02 17:52:29]

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

[wychen, 2015-11-02 17:52:31]

Could you take another look at this?

This CL has the following dependencies merged in, so please ignore them here.

The Blink part is at:
https://codereview.chromium.org/1419033004/
https://codereview.chromium.org/1415183004/

The print_preview part is at:
https://codereview.chromium.org/1419363004/

[mdjones, 2015-11-03 02:54:46]

A few nits.

https://codereview.chromium.org/1248643004/diff/300001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java
(right):

https://codereview.chromium.org/1248643004/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:84:
private boolean mSetCallback;
mIsCallbackSet; the current name feels like a function.

https://codereview.chromium.org/1248643004/diff/300001/components/dom_distill...
File components/dom_distiller/content/renderer/distillability_agent.cc (right):

https://codereview.chromium.org/1248643004/diff/300001/components/dom_distill...
components/dom_distiller/content/renderer/distillability_agent.cc:39: bool
isLast(bool is_loaded) {
Please add some doc for these helpers.

https://codereview.chromium.org/1248643004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp (right):

https://codereview.chromium.org/1248643004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:77: for (auto
word : {"banner", "combx", "comment", "community", "disqus", "extra", "foot",
"header", "menu", "related", "remark", "rss", "share", "shoutbox", "sidebar",
"skyscraper", "sponsor", "ad-break", "agegate", "pagination", "pager", "popup"})
{
Do webkit files not have the 80 char line limit?

[esprehn, 2015-11-03 04:12:10]

https://codereview.chromium.org/1248643004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp (right):

https://codereview.chromium.org/1248643004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:77: for (auto
word : {"banner", "combx", "comment", "community", "disqus", "extra", "foot",
"header", "menu", "related", "remark", "rss", "share", "shoutbox", "sidebar",
"skyscraper", "sponsor", "ad-break", "agegate", "pagination", "pager", "popup"})
{
On 2015/11/03 at 02:54:46, mdjones wrote:
> Do webkit files not have the 80 char line limit?

They do not, you should wrap where you think it's reasonable.

This line is probably a bit on the long side, I'd probably actually move this
array out of the loop and put one string per line.

[esprehn, 2015-11-03 07:20:20]

On 2015/11/03 at 04:12:10, esprehn wrote:
>
https://codereview.chromium.org/1248643004/diff/300001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp
(right):
> 
>
https://codereview.chromium.org/1248643004/diff/300001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:77: for
(auto word : {"banner", "combx", "comment", "community", "disqus", "extra",
"foot", "header", "menu", "related", "remark", "rss", "share", "shoutbox",
"sidebar", "skyscraper", "sponsor", "ad-break", "agegate", "pagination",
"pager", "popup"}) {
> On 2015/11/03 at 02:54:46, mdjones wrote:
> > Do webkit files not have the 80 char line limit?
> 
> They do not, you should wrap where you think it's reasonable.
> 
> This line is probably a bit on the long side, I'd probably actually move this
array out of the loop and put one string per line.

ex. of a big tables in blink:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[wychen, 2015-11-03 07:55:47]

Patchset #16 (id:320001) has been deleted

[wychen, 2015-11-03 07:56:39]

https://codereview.chromium.org/1248643004/diff/300001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java
(right):

https://codereview.chromium.org/1248643004/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:84:
private boolean mSetCallback;
On 2015/11/03 02:54:45, mdjones wrote:
> mIsCallbackSet; the current name feels like a function.

Done.

https://codereview.chromium.org/1248643004/diff/300001/components/dom_distill...
File components/dom_distiller/content/renderer/distillability_agent.cc (right):

https://codereview.chromium.org/1248643004/diff/300001/components/dom_distill...
components/dom_distiller/content/renderer/distillability_agent.cc:39: bool
isLast(bool is_loaded) {
On 2015/11/03 02:54:45, mdjones wrote:
> Please add some doc for these helpers.

Done.

https://codereview.chromium.org/1248643004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp (right):

https://codereview.chromium.org/1248643004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentStatisticsCollector.cpp:77: for (auto
word : {"banner", "combx", "comment", "community", "disqus", "extra", "foot",
"header", "menu", "related", "remark", "rss", "share", "shoutbox", "sidebar",
"skyscraper", "sponsor", "ad-break", "agegate", "pagination", "pager", "popup"})
{
On 2015/11/03 04:12:10, esprehn wrote:
> On 2015/11/03 at 02:54:46, mdjones wrote:
> > Do webkit files not have the 80 char line limit?
> 
> They do not, you should wrap where you think it's reasonable.
> 
> This line is probably a bit on the long side, I'd probably actually move this
> array out of the loop and put one string per line.

Fixed here:
https://codereview.chromium.org/1419033004/#ps250001

Sorry for pulling all the depended CLs here.

[esprehn, 2015-11-04 00:52:03]

This patch is so massive, I don't think we want to land something this big.

[wychen, 2015-11-04 01:37:20]

On 2015/11/04 00:52:03, esprehn wrote:
> This patch is so massive, I don't think we want to land something this big.

I agree. The WebKit part is in another CL already, so is the print_preview.

For the rest of it, I plan to land a CL that creates the new distillability
mechanism, and remove it in another.
This would roughly cut the size in half, so it's easier to review.

[wychen, 2015-11-04 01:55:02]

Patchset #18 (id:380001) has been deleted

[wychen, 2015-11-04 03:40:01]

Description was changed from

==========
Test distillability without JavaScript

In order to save memory usage, a native version of feature extraction is used
for distillability test. The features are inspired by the original
extract_features.js code.

Some tests are moved from components_browsertests to browser_tests because
DistillabilityAgent() is created in chrome layer renderer.

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

to

==========
Test distillability without JavaScript

In order to save memory usage, a native version of feature extraction is used
for distillability test. The features are inspired by the original
extract_features.js code.

Some tests are moved from components_browsertests to browser_tests because
DistillabilityAgent() is created in chrome layer renderer.

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

[wychen, 2015-11-04 04:37:04]

Description was changed from

==========
Test distillability without JavaScript

In order to save memory usage, a native version of feature extraction is used
for distillability test. The features are inspired by the original
extract_features.js code.

Some tests are moved from components_browsertests to browser_tests because
DistillabilityAgent() is created in chrome layer renderer.

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

to

==========
Test distillability without JavaScript

In order to save memory usage, a native version of feature extraction is used
for distillability test. The features are inspired by the original
extract_features.js code.

Some tests are moved from components_browsertests to browser_tests because
DistillabilityAgent() is created in chrome layer renderer.

BUG=509869
TEST=browser_tests --gtest_filter="Distillable*"
TEST=components_browsertests
--gtest_filter="DomDistillerDistillablePageUtilsTest.*"
TEST=components_unittests --gtest_filter="DomDistillerPageFeaturesTest*"
==========

[wychen, 2015-11-04 19:11:33]

On 2015/11/04 01:37:20, wychen wrote:
> On 2015/11/04 00:52:03, esprehn wrote:
> > This patch is so massive, I don't think we want to land something this big.
> 
> I agree. The WebKit part is in another CL already, so is the print_preview.
> 
> For the rest of it, I plan to land a CL that creates the new distillability
> mechanism, and remove it in another.
> This would roughly cut the size in half, so it's easier to review.

The CL is split to a series of smaller CLs here:
https://codereview.chromium.org/1409133007/
https://codereview.chromium.org/1405233009/
https://codereview.chromium.org/1434433002/
https://codereview.chromium.org/1414283006/
https://codereview.chromium.org/1422023005/

[nyquist, 2015-11-08 15:15:00]

Thanks for splitting this up. Feel free to add me to the other reviews if you
believe I could provide useful feedback.

[dglazkov, 2016-02-05 22:03:16]

Are we ready to close this issue?