Use new DomDistillerJs Proto API

Use new DomDistillerJs Proto API

This updates the distiller_page to use the proto-based
DomDistiller.applyWithOptions().

To provide options, domdistiller.js contains a $$OPTIONS placeholder
that is replaced with the json-serialized DomDistillerOptions at
runtime.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270538

[cjhopman, 2014-05-09 00:19:59]

yfriedman: *

[Yaron, 2014-05-09 01:00:55]

https://codereview.chromium.org/270663005/diff/40001/components/dom_distiller...
File components/dom_distiller/DEPS (right):

https://codereview.chromium.org/270663005/diff/40001/components/dom_distiller...
components/dom_distiller/DEPS:2: "+dom_distiller_js",
not third_party/dom_distiller_js?

https://codereview.chromium.org/270663005/diff/40001/components/dom_distiller...
File components/dom_distiller/core/distiller_page.cc (right):

https://codereview.chromium.org/270663005/diff/40001/components/dom_distiller...
components/dom_distiller/core/distiller_page.cc:26:
dom_distiller::proto::DomDistillerOptions options) {
const&

https://codereview.chromium.org/270663005/diff/40001/components/dom_distiller...
components/dom_distiller/core/distiller_page.cc:36: size_t options_offset =
script.find(kOptionsPlaceholder);
Instead of rolling your own, why not use base::ReplaceStringPlaceholders?

https://codereview.chromium.org/270663005/diff/40001/components/dom_distiller...
File components/dom_distiller/core/javascript/domdistiller.js (right):

https://codereview.chromium.org/270663005/diff/40001/components/dom_distiller...
components/dom_distiller/core/javascript/domdistiller.js:14: res =
com.dom_distiller.DomDistiller.applyWithOptions($$OPTIONS);
jsut inline in the return (or add a "var" if you want this around for debugging)

https://codereview.chromium.org/270663005/diff/40001/third_party/dom_distille...
File third_party/dom_distiller_js/dom_distiller_js.gyp (right):

https://codereview.chromium.org/270663005/diff/40001/third_party/dom_distille...
third_party/dom_distiller_js/dom_distiller_js.gyp:1: {
copyright

https://codereview.chromium.org/270663005/diff/40001/third_party/dom_distille...
third_party/dom_distiller_js/dom_distiller_js.gyp:9: 'proto_out_dir':
'dom_distiller_js',
third_party/dom_distiller_js ?

[cjhopman, 2014-05-09 01:55:05]

https://codereview.chromium.org/270663005/diff/40001/components/dom_distiller...
File components/dom_distiller/DEPS (right):

https://codereview.chromium.org/270663005/diff/40001/components/dom_distiller...
components/dom_distiller/DEPS:2: "+dom_distiller_js",
On 2014/05/09 01:00:55, Yaron wrote:
> not third_party/dom_distiller_js?

Done.

https://codereview.chromium.org/270663005/diff/40001/components/dom_distiller...
File components/dom_distiller/core/distiller_page.cc (right):

https://codereview.chromium.org/270663005/diff/40001/components/dom_distiller...
components/dom_distiller/core/distiller_page.cc:26:
dom_distiller::proto::DomDistillerOptions options) {
On 2014/05/09 01:00:55, Yaron wrote:
> const&

Done.

https://codereview.chromium.org/270663005/diff/40001/components/dom_distiller...
components/dom_distiller/core/distiller_page.cc:36: size_t options_offset =
script.find(kOptionsPlaceholder);
On 2014/05/09 01:00:55, Yaron wrote:
> Instead of rolling your own, why not use base::ReplaceStringPlaceholders?
ReplaceStringPlaceholders is rather dangerous, it will just blindly replace all
instances of $1 and $$ with the options and $ respectively. We can't really
ensure that those strings don't appear in the generated javascript (and when I
tried using ReplaceStringPlaceholders they did appear and it led to really
confusing javascript errors).

https://codereview.chromium.org/270663005/diff/40001/third_party/dom_distille...
File third_party/dom_distiller_js/dom_distiller_js.gyp (right):

https://codereview.chromium.org/270663005/diff/40001/third_party/dom_distille...
third_party/dom_distiller_js/dom_distiller_js.gyp:1: {
On 2014/05/09 01:00:55, Yaron wrote:
> copyright

Done.

https://codereview.chromium.org/270663005/diff/40001/third_party/dom_distille...
third_party/dom_distiller_js/dom_distiller_js.gyp:9: 'proto_out_dir':
'dom_distiller_js',
On 2014/05/09 01:00:55, Yaron wrote:
> third_party/dom_distiller_js ?

Done.

https://codereview.chromium.org/270663005/diff/60001/third_party/dom_distille...
File
third_party/dom_distiller_js/package/proto_gen/third_party/dom_distiller_js/dom_distiller_json_converter.h
(right):

https://codereview.chromium.org/270663005/diff/60001/third_party/dom_distille...
third_party/dom_distiller_js/package/proto_gen/third_party/dom_distiller_js/dom_distiller_json_converter.h:1:
#include "third_party/dom_distiller_js/dom_distiller.pb.h"
I just moved this and edited it in the repo for this change, but the generation
in DomDistillerJs will need to be updated/fixed to match this.

[Yaron, 2014-05-09 02:01:31]

lgtm

[cjhopman, 2014-05-13 16:44:49]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-05-13 16:45:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cjhopman@chromium.org/270663005/60001

[commit-bot: I haz the power, 2014-05-13 17:14:10]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-13 17:17:35]

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

[commit-bot: I haz the power, 2014-05-13 17:17:36]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[cjhopman, 2014-05-13 17:22:47]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-05-13 17:23:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cjhopman@chromium.org/270663005/80001

[commit-bot: I haz the power, 2014-05-13 20:25:53]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-13 20:54:46]

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

[commit-bot: I haz the power, 2014-05-13 20:54:46]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[cjhopman, 2014-05-14 16:15:09]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-05-14 16:15:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cjhopman@chromium.org/270663005/100001

[commit-bot: I haz the power, 2014-05-14 17:49:31]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-14 17:52:05]

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

[commit-bot: I haz the power, 2014-05-14 17:52:06]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)

[cjhopman, 2014-05-14 20:05:14]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-05-14 20:06:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cjhopman@chromium.org/270663005/120001

[commit-bot: I haz the power, 2014-05-14 23:30:58]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-15 00:33:04]

Change committed as 270538