Pass more information by argument to the dom_distiller script.

Pass more information by argument to the dom_distiller script.

BUG=None

Committed: https://crrev.com/5bc9087dbd77c67fba661c81df8f88aa2733b1f8
Cr-Commit-Position: refs/heads/master@{#320899}

[noyau (Ping after 24h), 2015-03-13 16:02:39]

Patchset #1 (id:1) has been deleted

[noyau (Ping after 24h), 2015-03-13 16:04:20]

noyau@chromium.org changed reviewers:
+ cjhopman@chromium.org, nyquist@chromium.org

[noyau (Ping after 24h), 2015-03-13 16:04:21]

Instead of trying to figure out what platform the code is running on by code, a
better approach is maybe to pass more arguments.

What do you think?

[nyquist, 2015-03-13 23:10:42]

On 2015/03/13 at 16:04:21, noyau wrote:
> Instead of trying to figure out what platform the code is running on by code,
a better approach is maybe to pass more arguments.
> 
> What do you think?

For what the code is doing, this looks reasonable.

Another suggestion for this would possibly be to just make all the code use the
iOS approach? I believe it should be supported on both. Any thoughts on that?

[noyau (Ping after 24h), 2015-03-16 10:05:12]

On 2015/03/13 23:10:42, nyquist wrote:
> On 2015/03/13 at 16:04:21, noyau wrote:
> > Instead of trying to figure out what platform the code is running on by
code,
> a better approach is maybe to pass more arguments.
> > 
> > What do you think?
> 
> For what the code is doing, this looks reasonable.
> 
> Another suggestion for this would possibly be to just make all the code use
the
> iOS approach? I believe it should be supported on both. Any thoughts on that?

Your call. I don't care one way or another. I just like this fixed, so there is
only one file.

[nyquist, 2015-03-16 20:39:03]

lgtm

I would like it better if this file lived in the external repository though.

Are there any plans for making components_browsertests work for iOS?

https://codereview.chromium.org/1004223002/diff/20001/components/dom_distille...
File components/dom_distiller/core/distiller_page.h (right):

https://codereview.chromium.org/1004223002/diff/20001/components/dom_distille...
components/dom_distiller/core/distiller_page.h:58: // The value returned between
the javascript and the DistillerPage can be
Nit: JavaScript here and below.

https://codereview.chromium.org/1004223002/diff/20001/components/dom_distille...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/1004223002/diff/20001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.mm:55: // UIWebView requires
javascript to return a single string value.
Nit: JavaScript

[noyau (Ping after 24h), 2015-03-17 10:35:39]

Feel free to move it in the external repository, I have no issue with that.

As for components_browsertests there is no way they will work on iOS, they 100%
depends on content. On iOS we do have integration tests, called KIF tests, but
they are really different from browsertests.

Ideally the javascript/page integration should be covered by a unittest.

https://codereview.chromium.org/1004223002/diff/20001/components/dom_distille...
File components/dom_distiller/core/distiller_page.h (right):

https://codereview.chromium.org/1004223002/diff/20001/components/dom_distille...
components/dom_distiller/core/distiller_page.h:58: // The value returned between
the javascript and the DistillerPage can be
On 2015/03/16 20:39:03, nyquist wrote:
> Nit: JavaScript here and below.

Done.

https://codereview.chromium.org/1004223002/diff/20001/components/dom_distille...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/1004223002/diff/20001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.mm:55: // UIWebView requires
javascript to return a single string value.
On 2015/03/16 20:39:03, nyquist wrote:
> Nit: JavaScript

Done.

[noyau (Ping after 24h), 2015-03-17 10:35:48]

The CQ bit was checked by noyau@chromium.org

[noyau (Ping after 24h), 2015-03-17 10:35:49]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1004223002/#ps40001
(title: "s/javascript/JavaScript")

[commit-bot: I haz the power, 2015-03-17 10:36:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1004223002/40001

[commit-bot: I haz the power, 2015-03-17 11:51:07]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-03-17 11:51:37]

Patchset 2 (id:??) landed as
https://crrev.com/5bc9087dbd77c67fba661c81df8f88aa2733b1f8
Cr-Commit-Position: refs/heads/master@{#320899}