Moves mojo_js_integration_tests into blink.

Moves mojo_js_integration_tests into blink.

BUG=699569
Review-Url: https://codereview.chromium.org/2734783008
Cr-Commit-Position: refs/heads/master@{#477549}
Committed: https://chromium.googlesource.com/chromium/src/+/dd57fa764813de09d159a232eeef1bb21b0ccc1e

[alokp, 2017-06-02 23:38:54]

Description was changed from

==========
Moves mojo_js_integration_tests into blink.

BUG=647036
==========

to

==========
Moves mojo_js_integration_tests into blink.

BUG=647036
==========

[alokp, 2017-06-02 23:38:55]

alokp@chromium.org changed reviewers:
+ yzshen@chromium.org

[alokp, 2017-06-02 23:49:58]

https://codereview.chromium.org/2734783008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp (right):

https://codereview.chromium.org/2734783008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:86: MojoResult result
= Wait(message_pipe_handle, MOJO_HANDLE_SIGNAL_READABLE);
result == MOJO_RESULT_INVALID_ARGUMENT here indicating that message_pipe_handle
received from js-side is invalid.

https://codereview.chromium.org/2734783008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/tests/JsToCppTest.js (right):

https://codereview.chromium.org/2734783008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.js:55: arg.data_handle =
dataPipe1.consumer;
JsToCppTest::Echo is failing because the message/data pipe handles serialized
into EchoArg is invalid when it is received on the cpp side. I suspect that this
is because the handle type is now MojoHandle. Do we need to change encode/decode
scheme for handles?

[alokp, 2017-06-02 23:51:31]

Description was changed from

==========
Moves mojo_js_integration_tests into blink.

BUG=647036
==========

to

==========
Moves mojo_js_integration_tests into blink.

BUG=699569
==========

[alokp, 2017-06-04 00:01:00]

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

[commit-bot: I haz the power, 2017-06-04 00:01:03]

Dry run: CQ is trying da patch.

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

[alokp, 2017-06-04 00:05:48]

alokp@chromium.org changed reviewers:
+ jbroman@chromium.org

[alokp, 2017-06-04 00:05:49]

I figured out the problem. JS-side was writing to incorrect member variables for
EchoArgs because the new bindings have changed the variable naming scheme from
foo_bar to fooBar.

This is ready for review.

[commit-bot: I haz the power, 2017-06-04 00:09:19]

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

[commit-bot: I haz the power, 2017-06-04 00:09:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbroman, 2017-06-05 15:53:06]

On 2017/06/04 at 00:05:49, alokp wrote:
> I figured out the problem. JS-side was writing to incorrect member variables
for EchoArgs because the new bindings have changed the variable naming scheme
from foo_bar to fooBar.
> 
> This is ready for review.

Several unit tests seem to be failing:

JsToCppTest.BackPointer
JsToCppTest.BitFlip
JsToCppTest.Ping
JsToCppTest.Echo

Mind looking at that before I do a full review?

[alokp, 2017-06-05 17:26:26]

On 2017/06/05 15:53:06, jbroman wrote:
> On 2017/06/04 at 00:05:49, alokp wrote:
> > I figured out the problem. JS-side was writing to incorrect member variables
> for EchoArgs because the new bindings have changed the variable naming scheme
> from foo_bar to fooBar.
> > 
> > This is ready for review.
> 
> Several unit tests seem to be failing:
> 
> JsToCppTest.BackPointer
> JsToCppTest.BitFlip
> JsToCppTest.Ping
> JsToCppTest.Echo
> 
> Mind looking at that before I do a full review?

Yeah please hold off the review. I noticed try failures on mac and win after
sending it out for review. It worked fine on linux. I will ping back after they
are passing everywhere.

[yzshen1, 2017-06-05 17:33:26]

LGTM with a few nits.

Thanks!

https://codereview.chromium.org/2734783008/diff/130013/mojo/edk/js/tests/BUIL...
File mojo/edk/js/tests/BUILD.gn (left):

https://codereview.chromium.org/2734783008/diff/130013/mojo/edk/js/tests/BUIL...
mojo/edk/js/tests/BUILD.gn:23: test("mojo_js_integration_tests") {
This is a bot failure:
"""{
  "error": "Invalid targets",
  "invalid_targets": [
    "//mojo/edk/js/tests:mojo_js_integration_tests"
  ]
}"""

Did you forget to remove references to this target?

https://codereview.chromium.org/2734783008/diff/130013/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp (right):

https://codereview.chromium.org/2734783008/diff/130013/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:52:
filepath.append("/gen/mojo/public/js/mojo_bindings.js");
Just to double check: is this accessible in isolated testing environment.

https://codereview.chromium.org/2734783008/diff/130013/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:72: MojoResult result
= BeginReadDataRaw(data_pipe_handle, &buffer, &num_bytes,
- Do we need to wait just like what line 85 does?
- It probably would never fail in practice, but please note that there is no
guarantee by the API that the data is able to be read out with a single call.

https://codereview.chromium.org/2734783008/diff/130013/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:210: :
js_side_(nullptr), mishandled_messages_(0), binding_(this) {}
nit: js_side_ init is not necessary.

[alokp, 2017-06-05 18:49:12]

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

[commit-bot: I haz the power, 2017-06-05 18:49:54]

Dry run: CQ is trying da patch.

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

[alokp, 2017-06-05 18:53:28]

alokp@chromium.org changed reviewers:
+ dpranke@chromium.org

[alokp, 2017-06-05 18:53:29]

Dirk: testing/ and one question for you in comments about datadeps. Thanks!

Jeremy: I am expecting the tests to pass now on all platforms. Because of a
missing datadeps, JS bindings for JsToCpp.mojom was missing.

https://codereview.chromium.org/2734783008/diff/130013/mojo/edk/js/tests/BUIL...
File mojo/edk/js/tests/BUILD.gn (left):

https://codereview.chromium.org/2734783008/diff/130013/mojo/edk/js/tests/BUIL...
mojo/edk/js/tests/BUILD.gn:23: test("mojo_js_integration_tests") {
On 2017/06/05 17:33:25, yzshen1 wrote:
> This is a bot failure:
> """{
>   "error": "Invalid targets",
>   "invalid_targets": [
>     "//mojo/edk/js/tests:mojo_js_integration_tests"
>   ]
> }"""
> 
> Did you forget to remove references to this target?

I had removed all references to this target in GN files. I have now removed all
references (that I could find) in various json and pyl files that are used on
bots. It would be helpful if the error message provided reference locations in
the log.

https://codereview.chromium.org/2734783008/diff/130013/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp (right):

https://codereview.chromium.org/2734783008/diff/130013/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:52:
filepath.append("/gen/mojo/public/js/mojo_bindings.js");
On 2017/06/05 17:33:26, yzshen1 wrote:
> Just to double check: is this accessible in isolated testing environment.

I am not sure. I am hoping that we include everything in GN target runtime_deps.
Including Dirk to comment.

https://codereview.chromium.org/2734783008/diff/130013/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:72: MojoResult result
= BeginReadDataRaw(data_pipe_handle, &buffer, &num_bytes,
On 2017/06/05 17:33:26, yzshen1 wrote:
> - Do we need to wait just like what line 85 does?
> - It probably would never fail in practice, but please note that there is no
> guarantee by the API that the data is able to be read out with a single call.
> 

Done.

https://codereview.chromium.org/2734783008/diff/130013/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:210: :
js_side_(nullptr), mishandled_messages_(0), binding_(this) {}
On 2017/06/05 17:33:25, yzshen1 wrote:
> nit: js_side_ init is not necessary.

Done.

[commit-bot: I haz the power, 2017-06-05 23:00:06]

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

[commit-bot: I haz the power, 2017-06-05 23:00:07]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-06-06 20:14:08]

lgtm modulo the below comments and the ones already raised

https://codereview.chromium.org/2734783008/diff/130013/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp (right):

https://codereview.chromium.org/2734783008/diff/130013/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:366:
page_holder_->GetPage().GetSettings().SetScriptEnabled(true);
V8TestingScope will take out some of the boilerplate here; it provides
convenient access to the objects of interest for JS execution.

https://codereview.chromium.org/2734783008/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp (right):

https://codereview.chromium.org/2734783008/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:366: JsToCppTest() :
page_holder_(DummyPageHolder::Create()) {
Does V8TestingScope work? It might trim out some of the boilerplate here (with
managing the PageHolder, Frame, ScriptState, etc.).

https://codereview.chromium.org/2734783008/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:397: args,
script_state->GetIsolate());
nit: prefer WTF_ARRAY_LENGTH(args) to 1. This also makes it more reasonable to
infer the length of args in the previous statement.

https://codereview.chromium.org/2734783008/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/tests/OWNERS (right):

https://codereview.chromium.org/2734783008/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/OWNERS:2: per-file
*.mojom=file://ipc/SECURITY_OWNERS
[cc rockot, dcheng]

This seems to be going opposite the PRESUBMIT.py change in rockot@'s CL:
https://chromium-review.googlesource.com/c/517357/

We should probably be consistent one way or the other about whether these Test
mojoms require security owner review.

[alokp, 2017-06-06 23:52:23]

https://codereview.chromium.org/2734783008/diff/130013/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp (right):

https://codereview.chromium.org/2734783008/diff/130013/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:366:
page_holder_->GetPage().GetSettings().SetScriptEnabled(true);
On 2017/06/06 20:14:08, jbroman wrote:
> V8TestingScope will take out some of the boilerplate here; it provides
> convenient access to the objects of interest for JS execution.

Done.

https://codereview.chromium.org/2734783008/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp (right):

https://codereview.chromium.org/2734783008/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:366: JsToCppTest() :
page_holder_(DummyPageHolder::Create()) {
On 2017/06/06 20:14:08, jbroman wrote:
> Does V8TestingScope work? It might trim out some of the boilerplate here (with
> managing the PageHolder, Frame, ScriptState, etc.).

Done.

https://codereview.chromium.org/2734783008/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/tests/JsToCppTest.cpp:397: args,
script_state->GetIsolate());
On 2017/06/06 20:14:08, jbroman wrote:
> nit: prefer WTF_ARRAY_LENGTH(args) to 1. This also makes it more reasonable to
> infer the length of args in the previous statement.

Done.

[alokp, 2017-06-06 23:53:30]

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

[commit-bot: I haz the power, 2017-06-06 23:54:10]

Dry run: CQ is trying da patch.

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

[alokp, 2017-06-06 23:54:27]

dpranke: testing/

[Dirk Pranke, 2017-06-07 01:14:13]

lgtm

[commit-bot: I haz the power, 2017-06-07 01:50:03]

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

[commit-bot: I haz the power, 2017-06-07 01:50:04]

Dry run: This issue passed the CQ dry run.

[alokp, 2017-06-07 04:21:57]

The CQ bit was checked by alokp@chromium.org

[alokp, 2017-06-07 04:21:58]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2734783008/#ps190001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-06-07 04:22:09]

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, 2017-06-07 04:27:04]

CQ is committing da patch.

Bot data: {"patchset_id": 190001, "attempt_start_ts": 1496809317185460,
"parent_rev": "8709f78a5a70fd9db4414ca6c0fe281d668f31e5", "commit_rev":
"dd57fa764813de09d159a232eeef1bb21b0ccc1e"}

[commit-bot: I haz the power, 2017-06-07 04:27:13]

Description was changed from

==========
Moves mojo_js_integration_tests into blink.

BUG=699569
==========

to

==========
Moves mojo_js_integration_tests into blink.

BUG=699569

Review-Url: https://codereview.chromium.org/2734783008
Cr-Commit-Position: refs/heads/master@{#477549}
Committed:
https://chromium.googlesource.com/chromium/src/+/dd57fa764813de09d159a232eeef...
==========

[commit-bot: I haz the power, 2017-06-07 04:27:15]

Committed patchset #11 (id:190001) as
https://chromium.googlesource.com/chromium/src/+/dd57fa764813de09d159a232eeef...

[jwd, 2017-06-07 15:20:00]

A revert of this CL (patchset #11 id:190001) has been created in
https://codereview.chromium.org/2926143002/ by jwd@chromium.org.

The reason for reverting is: Seems related to webkit failures of the JsToCpp
tests. E.x.
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build....