Fix domdistiller for new JS execution

Fix domdistiller for new JS execution

Javascript execution does not support returning strings anymore; fixed
domdistiller to work with this requirement.

BUG=NONE

Committed: https://crrev.com/3d08d5a050833c9f5f24cf5ab006b9675d6ea0ac
Cr-Commit-Position: refs/heads/master@{#419749}

[lody, 2016-09-09 09:23:56]

lod@chromium.org changed reviewers:
+ eugenebut@chromium.org, noyau@chromium.org

[lody, 2016-09-09 09:23:57]

As discussed w/ eugene this is duplicated code but is the safest way to modify
JS result parsing for this specific case.

[Eugene But (OOO till 7-30), 2016-09-09 15:28:28]

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
File components/dom_distiller/ios/distiller_page_ios.h (right):

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.h:49:
std::unique_ptr<base::Value> ValueResultFromWKResult(id wk_result);
Optional NIT: Maybe s/WKResult/ScriptResult for consistency with previous method
name?

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.h:49:
std::unique_ptr<base::Value> ValueResultFromWKResult(id wk_result);
Per Style Guide every non-trivial method needs comments.

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:123: return nullptr;
Optional NIT: You can return |base::Value::CreateNullValue()| here and drop
check on line 107

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:150: const std::string&
path([key UTF8String]);
Please use sys_string_conversions instead of |UTF8String|. I asked author of
original code to fix this as well.

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:152:
ValueResultFromWKResult([wk_result objectForKey:key]));
NIT: s/[wk_result objectForKey:key]/wk_result[key]

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:152:
ValueResultFromWKResult([wk_result objectForKey:key]));
Can this recursion cause a stack overflow?

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:158:
list->Append(ValueResultFromWKResult(value));
Same question here

[Eugene But (OOO till 7-30), 2016-09-10 00:23:40]

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:152:
ValueResultFromWKResult([wk_result objectForKey:key]));
On 2016/09/09 15:28:28, Eugene But wrote:
> NIT: s/[wk_result objectForKey:key]/wk_result[key]
JsonParser measures stack depth:
https://cs.chromium.org/chromium/src/base/json/json_parser.cc?q=StackMarker&s...

[lody, 2016-09-19 15:31:04]

Patchset #2 (id:20001) has been deleted

[lody, 2016-09-19 15:34:36]

eugene, are you ok finishing this CL? I saw you mentioned not taking on so many
CLs right now so let me know if i should assign to someone else, thanks.

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
File components/dom_distiller/ios/distiller_page_ios.h (right):

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.h:49:
std::unique_ptr<base::Value> ValueResultFromWKResult(id wk_result);
On 2016/09/09 15:28:28, Eugene But wrote:
> Per Style Guide every non-trivial method needs comments.

Done.

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
File components/dom_distiller/ios/distiller_page_ios.mm (right):

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:123: return nullptr;
On 2016/09/09 15:28:28, Eugene But wrote:
> Optional NIT: You can return |base::Value::CreateNullValue()| here and drop
> check on line 107

I'd rather not have any part of this be different from the original code except
what's necessary; it'll make it easier to read and update.

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:150: const std::string&
path([key UTF8String]);
On 2016/09/09 15:28:28, Eugene But wrote:
> Please use sys_string_conversions instead of |UTF8String|. I asked author of
> original code to fix this as well.

Done.

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:152:
ValueResultFromWKResult([wk_result objectForKey:key]));
On 2016/09/10 00:23:40, Eugene But wrote:
> On 2016/09/09 15:28:28, Eugene But wrote:
> > NIT: s/[wk_result objectForKey:key]/wk_result[key]
> JsonParser measures stack depth:
>
https://cs.chromium.org/chromium/src/base/json/json_parser.cc?q=StackMarker&s...
> 

Duplicated code fixed by jif.

https://codereview.chromium.org/2327783002/diff/1/components/dom_distiller/io...
components/dom_distiller/ios/distiller_page_ios.mm:158:
list->Append(ValueResultFromWKResult(value));
On 2016/09/09 15:28:28, Eugene But wrote:
> Same question here

Acknowledged.

[Eugene But (OOO till 7-30), 2016-09-19 21:52:27]

It would make sense to finish all reviews I started :) lgtm and thank you for
adding array handling code!

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

https://codereview.chromium.org/2327783002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.mm:25: // However WKWebView only
allows us to return "numbers." We need to create
NIT: Please avoid using "we" and generally pronouns. They are ambiguous.

https://codereview.chromium.org/2327783002/diff/40001/ios/web/web_state/ui/we...
File ios/web/web_state/ui/web_view_js_utils_unittest.mm (right):

https://codereview.chromium.org/2327783002/diff/40001/ios/web/web_state/ui/we...
ios/web/web_state/ui/web_view_js_utils_unittest.mm:98: TEST(WebViewJsUtilsTest,
ValueResultFromArrayWKResult) {
Could you please add test for stack smashing case (deep array).

https://codereview.chromium.org/2327783002/diff/40001/ios/web/web_state/ui/we...
ios/web/web_state/ui/web_view_js_utils_unittest.mm:113: base::ListValue const*
inner_list;
Please initialize

[lody, 2016-09-20 13:41:37]

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

https://codereview.chromium.org/2327783002/diff/40001/components/dom_distille...
components/dom_distiller/ios/distiller_page_ios.mm:25: // However WKWebView only
allows us to return "numbers." We need to create
On 2016/09/19 21:52:27, Eugene But wrote:
> NIT: Please avoid using "we" and generally pronouns. They are ambiguous.

Done.

https://codereview.chromium.org/2327783002/diff/40001/ios/web/web_state/ui/we...
File ios/web/web_state/ui/web_view_js_utils_unittest.mm (right):

https://codereview.chromium.org/2327783002/diff/40001/ios/web/web_state/ui/we...
ios/web/web_state/ui/web_view_js_utils_unittest.mm:98: TEST(WebViewJsUtilsTest,
ValueResultFromArrayWKResult) {
On 2016/09/19 21:52:27, Eugene But wrote:
> Could you please add test for stack smashing case (deep array).

Done, and test caught bug! (insert "is such a thing even possible" meme here ;)
)

https://codereview.chromium.org/2327783002/diff/40001/ios/web/web_state/ui/we...
ios/web/web_state/ui/web_view_js_utils_unittest.mm:113: base::ListValue const*
inner_list;
On 2016/09/19 21:52:27, Eugene But wrote:
> Please initialize

Done.

[noyau (Ping after 24h), 2016-09-20 13:52:50]

components/dom_distiller/ios/ lgtm.

[lody, 2016-09-20 13:53:04]

The CQ bit was checked by lod@chromium.org

[lody, 2016-09-20 13:53:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2327783002/#ps60001
(title: "test & comment")

[commit-bot: I haz the power, 2016-09-20 13:53:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 14:09:26]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-09-20 14:10:53]

Description was changed from

==========
Fix domdistiller for new JS execution

Javascript execution does not support returning strings anymore; fixed
domdistiller to work with this requirement.

BUG=NONE
==========

to

==========
Fix domdistiller for new JS execution

Javascript execution does not support returning strings anymore; fixed
domdistiller to work with this requirement.

BUG=NONE

Committed: https://crrev.com/3d08d5a050833c9f5f24cf5ab006b9675d6ea0ac
Cr-Commit-Position: refs/heads/master@{#419749}
==========

[commit-bot: I haz the power, 2016-09-20 14:10:55]

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