Add interface versioning. Methods queryVersion and requireVersion.

Add interface versioning for javascript.

Methods queryVersion and requireVersion are added to InterfacePtrController.
Support sending and handling control messages by creating control_message_proxy
and control_message_handler. control_message_proxy uses promises as opposed to
callbacks which is what our original cpp code does.

BUG=684879
R=yzshen@chromium.org

Review-Url: https://codereview.chromium.org/2676443005
Cr-Commit-Position: refs/heads/master@{#450488}
Committed: https://chromium.googlesource.com/chromium/src/+/43ca78b29ea7366bbbc35e28a272fb145771181f

[wangjimmy, 2017-02-04 01:32:42]

https://codereview.chromium.org/2676443005/diff/1/mojo/public/js/lib/control_...
File mojo/public/js/lib/control_message_proxy.js (right):

https://codereview.chromium.org/2676443005/diff/1/mojo/public/js/lib/control_...
mojo/public/js/lib/control_message_proxy.js:59: return
receiver.acceptAndExpectResponse(message).then(
Router.js 's acceptAndExpectResponse never resolves. Run mojo_js_unittests,
JSTest.InterfacePtr. To see queryVersion get called and which calls this method.

[yzshen1, 2017-02-06 06:57:26]

https://codereview.chromium.org/2676443005/diff/1/mojo/public/js/tests/interf...
File mojo/public/js/tests/interface_ptr_unittest.js (right):

https://codereview.chromium.org/2676443005/diff/1/mojo/public/js/tests/interf...
mojo/public/js/tests/interface_ptr_unittest.js:165: var integerAccessorImpl =
new sampleInterfaces.IntegerAccessorPtr();
This is not an "implementation". An implementation is what you use to provide
the service. You using the Binding class to connect an implementation with an
interface request. Here what you create is the interface pointer, i.e., the user
side. Please look at line 112 - 113 as an example. We could talk more in-person.
These basic concepts are pretty important.

https://codereview.chromium.org/2676443005/diff/1/mojo/public/js/tests/interf...
mojo/public/js/tests/interface_ptr_unittest.js:166: var request =
bindings.makeRequest(integerAccessorImpl);
The problem is that you need to provide support for the receiving side to handle
the control message. You will need to do:
- implement equivalent of
mojo/public/cpp/bindings/lib/control_message_handler.h, whenever Router receives
a message, it needs to check whether it is a control message, if yes, it lets
the control message handler handles it instead.
- here in this test, you need to bind this |request| to an Binding.

[yzshen1, 2017-02-06 07:04:19]

https://codereview.chromium.org/2676443005/diff/1/mojo/public/js/tests/interf...
File mojo/public/js/tests/interface_ptr_unittest.js (right):

https://codereview.chromium.org/2676443005/diff/1/mojo/public/js/tests/interf...
mojo/public/js/tests/interface_ptr_unittest.js:165: var integerAccessorImpl =
new sampleInterfaces.IntegerAccessorPtr();
On 2017/02/06 06:57:26, yzshen1 wrote:
> This is not an "implementation". An implementation is what you use to provide
> the service. You using the Binding class to connect an implementation with an
> interface request.
Sorry, I meant to say "You should use the Binding class ..."

> Here what you create is the interface pointer, i.e., the user
> side. Please look at line 112 - 113 as an example. We could talk more
in-person.
> These basic concepts are pretty important.

[wangjimmy, 2017-02-09 06:54:54]

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

[commit-bot: I haz the power, 2017-02-09 06:55:20]

Dry run: 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-02-09 07:42:40]

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

[commit-bot: I haz the power, 2017-02-09 07:42:41]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[wangjimmy, 2017-02-10 00:20:12]

Description was changed from

==========
Add interface versioning. Methods queryVersion and requireVersion.

BUG=684879
R=yzshen@chromium.org
==========

to

==========
Add interface versioning for javascript.

Methods queryVersion and requireVersion are added to InterfacePtrController.
Support sending and handling control messages by creating control_message_proxy
and control_message_handler. control_message_proxy uses promises as opposed to
callbacks which is what our original cpp code does.

BUG=684879
R=yzshen@chromium.org
==========

[wangjimmy, 2017-02-10 00:21:18]

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

[commit-bot: I haz the power, 2017-02-10 00:22:08]

Dry run: 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-02-10 01:03:31]

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

[commit-bot: I haz the power, 2017-02-10 01:03:33]

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

[wangjimmy, 2017-02-10 19:59:01]

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

[commit-bot: I haz the power, 2017-02-10 19:59:56]

Dry run: 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-02-10 20:10:01]

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

[commit-bot: I haz the power, 2017-02-10 20:10:02]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[yzshen1, 2017-02-11 01:11:30]

Sorry if I seem to be picky. :) It is normal that first few CLs of new
contributors get a lot of comments. That is helpful for them to get used to the
coding style and get familiar with the codebase.

I think this patch mostly looks good.

https://codereview.chromium.org/2676443005/diff/60001/content/content_resourc...
File content/content_resources.grd (right):

https://codereview.chromium.org/2676443005/diff/60001/content/content_resourc...
content/content_resources.grd:60: <include
name="IDR_MOJO_CONTROL_MESSAGE_PROXY_JS"
file="../mojo/public/js/lib/control_message_proxy.js" flattenhtml="true"
type="BINDATA" />
Please order these js-bindings files by their names alphabetically. (here and
also other files, such as mojo_context_state.cc, dispatcher.cc, constants, etc.)

You don't have to reorder the entire file, just within the group of js-bindings
files.

https://codereview.chromium.org/2676443005/diff/60001/content/content_resourc...
content/content_resources.grd:62: <include
name="IDR_MOJO_INTERFACE_CONTROL_MESSAGES_MOJO_JS"
file="../mojo/public/interfaces/bindings/interface_control_messages.mojom.js"
flattenhtml="true" type="BINDATA" />
Does it make sense to use "..._MOJOM_JS" instead of "..._MOJO_JS" to better
match the file name?

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/BUILD.gn
File mojo/public/js/BUILD.gn (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/BUILD.gn...
mojo/public/js/BUILD.gn:27: "lib/control_message_proxy.js",
Please order files in this list alphabetically.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/bindings.js
File mojo/public/js/bindings.js (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/bindings...
mojo/public/js/bindings.js:9: "mojo/public/js/lib/control_message_proxy",
alphabetically, please.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/bindings...
mojo/public/js/bindings.js:35: this.controlMessageProxy_ = new
controlMessageProxy.ControlMessageProxy();
Please consider setting the value to null and moving creation of new object into
configureProxyIfNecessary_().

Because one common operation is to create an interface pointer and pass it over
a message pipe. In that case, creating the controlMessageProxy_ here is a little
wasteful. And you could also pass receiver as a constructor parameter instead of
directly accessing the member.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/bindings...
mojo/public/js/bindings.js:134: if (this.version >= version) {
Whether to have {} around single-line body is not enforced by the style guide.
But it is nice to be consistent with the surrounding code. See line 109 for
example.

Another thing to be noted is that if-else should have {} for both blocks or none
of them.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/bindings...
mojo/public/js/bindings.js:186: this.router_ = new router.Router(handle,
this.interfaceType_.version);
|version| should be named kVersion since it is a constant.

The corresponding field in C++ doesn't follow the style guide. It should really
be kVersion in C++, but it is named Version_ for historical reasons.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
File mojo/public/js/lib/control_message_handler.js (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:7: "mojo/public/js/validator",
order the list alphabetically, please.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:9: ], function(interfaceCtrlMsgs,
validator, codec) {
interfaceCtrlMsgs -> controlMessages ?

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:20: if (!message.getName() &&
!message.getName() seems wrong, I guess you mean message.getName()?
Do we need this check at all?

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:22: throw new Error('Control
message name is not kRunMessageId');
We also need to validate the payload of the message.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:22: throw new Error('Control
message name is not kRunMessageId');
We should probably use double quotes here (and below) to be consistent.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:33: if (!message.getName() &&
see comment above.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:35: throw new Error('Control
message name is not kRunOrClosePipeMessageId');
We also need to validate the payload.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:43: return interface_version >=
runOrClosePipeMsgParams.input
Formatting:

return interface_version >=
    runOrClosePipeMsgParams.input.require_version.version;

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:56:
interfaceCtrlMsgs.QueryVersionResult({'version': interface_version});
4 spaces.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:66: payloadSize,
codec.kMessageIsResponse, requestID);
4 spaces

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:78: function
ControlMessageHandler(responder, interface_version) {
Maybe we should remove responder from the constructor? Please also see my
comment on line 88.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:88:
ControlMessageHandler.prototype.acceptWithResponder = function(message) {
The calling code pass a responder to this method, please see router.js.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
File mojo/public/js/lib/control_message_proxy.js (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:7: "mojo/public/js/validator",
Alphabetically, please.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:9: ], function(interfaceCtrlMsgs,
validator, codec) {
interfaceCtrlMsgs -> controlMessages?

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:31: if (!message.getName() &&
Do we need this check at all?

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:33: throw new Error('Control message
name is not kRunMessageId');
Please consider using double quotes.

And we need to validate the payload as well.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:37: function accept(message) {
Could we give it a more descriptive name? We have too many methods named accept.
:)

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:59: // requestID is set to 0, but is
later
Please fill this line to as close to 80 chars as possible.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:60: // properly set by Router's
acceptAndExpectResponse
Usually in comments, we use xxx() to indicate function names and |xxx| to
indicate variables.

Comments should be sentence with trailing "."

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:79: runResponseMsgParams) {
4 spaces.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:86:
interfaceCtrlMsgs.RunOrClosePipeMessageParams();
4 spaces.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:89:
interfaceCtrlMsgs.RequireVersion({'version': version});
4 spaces.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/router.js
File mojo/public/js/router.js (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/router.j...
mojo/public/js/router.js:33: this.controlMsgHandler_ = new
ControlMessageHandler(this,
It is preferred to use full name: Msg -> Message. Unless it is something
commonly used such as Url, Http.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/router.j...
mojo/public/js/router.js:34: interface_version);
4 spaces. Please see:
https://google.github.io/styleguide/javascriptguide.xml?showone=Code_formatti...

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/router.j...
mojo/public/js/router.js:118: controlMessageHandler.isControlMessage(message)) {
if the message is a control message but we don't have a control message handler,
we should not pass this message to incomingReceiver_, instead we should directly
go to tear down the pipe.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/tests/in...
File mojo/public/js/tests/interface_ptr_unittest.js (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/tests/in...
mojo/public/js/tests/interface_ptr_unittest.js:181:
sampleInterfaces.IntegerAccessor,
4 spaces
please see:
https://google.github.io/styleguide/javascriptguide.xml?showone=Code_formatti...

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/tests/in...
mojo/public/js/tests/interface_ptr_unittest.js:184: );
");" should not be on a separate line.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/tests/in...
mojo/public/js/tests/interface_ptr_unittest.js:197:
sampleInterfaces.IntegerAccessor,
4 spaces.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/tests/in...
mojo/public/js/tests/interface_ptr_unittest.js:235:
expect(integerAccessorImpl.integer).toBe(456);
Because the call on line 234 is completed asynchronously, it won't change the
value at this point. In order to really test that it is ignored, you should move
this line into the connection error handler on 229.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/js_templates/module.amd.tmpl (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/module.amd.tmpl:6: {%-  if
module.path !=
"mojo/public/interfaces/bindings/interface_control_messages.mojom" %}
nit: we don't have style guide for tmpl files, but please be consistent with
surrounding code:

"{%- if" (only one space between "-" and "if")
or "{%  if" (two spaces).

Nested blocks:

{%- if ... %}
{%-   if ... %}
{%    endif %}
{%  endif %}

[wangjimmy, 2017-02-11 02:10:26]

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

[commit-bot: I haz the power, 2017-02-11 02:10:55]

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

[wangjimmy, 2017-02-11 07:29:43]

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

[commit-bot: I haz the power, 2017-02-11 07:30:02]

Dry run: 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-02-11 08:38:55]

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

[commit-bot: I haz the power, 2017-02-11 08:38:56]

Dry run: This issue passed the CQ dry run.

[wangjimmy, 2017-02-13 20:40:27]

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

[commit-bot: I haz the power, 2017-02-13 20:41:33]

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

[wangjimmy, 2017-02-13 20:43:08]

https://codereview.chromium.org/2676443005/diff/60001/content/content_resourc...
File content/content_resources.grd (right):

https://codereview.chromium.org/2676443005/diff/60001/content/content_resourc...
content/content_resources.grd:60: <include
name="IDR_MOJO_CONTROL_MESSAGE_PROXY_JS"
file="../mojo/public/js/lib/control_message_proxy.js" flattenhtml="true"
type="BINDATA" />
On 2017/02/11 01:11:28, yzshen1 wrote:
> Please order these js-bindings files by their names alphabetically. (here and
> also other files, such as mojo_context_state.cc, dispatcher.cc, constants,
etc.)
> 
> You don't have to reorder the entire file, just within the group of
js-bindings
> files.

Done.

https://codereview.chromium.org/2676443005/diff/60001/content/content_resourc...
content/content_resources.grd:62: <include
name="IDR_MOJO_INTERFACE_CONTROL_MESSAGES_MOJO_JS"
file="../mojo/public/interfaces/bindings/interface_control_messages.mojom.js"
flattenhtml="true" type="BINDATA" />
On 2017/02/11 01:11:28, yzshen1 wrote:
> Does it make sense to use "..._MOJOM_JS" instead of "..._MOJO_JS" to better
> match the file name?

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/BUILD.gn
File mojo/public/js/BUILD.gn (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/BUILD.gn...
mojo/public/js/BUILD.gn:27: "lib/control_message_proxy.js",
On 2017/02/11 01:11:28, yzshen1 wrote:
> Please order files in this list alphabetically.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/bindings.js
File mojo/public/js/bindings.js (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/bindings...
mojo/public/js/bindings.js:9: "mojo/public/js/lib/control_message_proxy",
On 2017/02/11 01:11:28, yzshen1 wrote:
> alphabetically, please.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/bindings...
mojo/public/js/bindings.js:35: this.controlMessageProxy_ = new
controlMessageProxy.ControlMessageProxy();
On 2017/02/11 01:11:28, yzshen1 wrote:
> Please consider setting the value to null and moving creation of new object
into
> configureProxyIfNecessary_().
> 
> Because one common operation is to create an interface pointer and pass it
over
> a message pipe. In that case, creating the controlMessageProxy_ here is a
little
> wasteful. And you could also pass receiver as a constructor parameter instead
of
> directly accessing the member.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/bindings...
mojo/public/js/bindings.js:134: if (this.version >= version) {
On 2017/02/11 01:11:28, yzshen1 wrote:
> Whether to have {} around single-line body is not enforced by the style guide.
> But it is nice to be consistent with the surrounding code. See line 109 for
> example.
> 
> Another thing to be noted is that if-else should have {} for both blocks or
none
> of them.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/bindings...
mojo/public/js/bindings.js:186: this.router_ = new router.Router(handle,
this.interfaceType_.version);
On 2017/02/11 01:11:28, yzshen1 wrote:
> |version| should be named kVersion since it is a constant.
> 
> The corresponding field in C++ doesn't follow the style guide. It should
really
> be kVersion in C++, but it is named Version_ for historical reasons.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
File mojo/public/js/lib/control_message_handler.js (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:7: "mojo/public/js/validator",
On 2017/02/11 01:11:29, yzshen1 wrote:
> order the list alphabetically, please.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:9: ], function(interfaceCtrlMsgs,
validator, codec) {
On 2017/02/11 01:11:29, yzshen1 wrote:
> interfaceCtrlMsgs -> controlMessages ?

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:20: if (!message.getName() &&
On 2017/02/11 01:11:29, yzshen1 wrote:
> !message.getName() seems wrong, I guess you mean message.getName()?
> Do we need this check at all?

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:22: throw new Error('Control
message name is not kRunMessageId');
On 2017/02/11 01:11:29, yzshen1 wrote:
> We should probably use double quotes here (and below) to be consistent.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:22: throw new Error('Control
message name is not kRunMessageId');
On 2017/02/11 01:11:29, yzshen1 wrote:
> We also need to validate the payload of the message. 

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:33: if (!message.getName() &&
On 2017/02/11 01:11:29, yzshen1 wrote:
> see comment above.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:35: throw new Error('Control
message name is not kRunOrClosePipeMessageId');
On 2017/02/11 01:11:28, yzshen1 wrote:
> We also need to validate the payload.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:43: return interface_version >=
runOrClosePipeMsgParams.input
On 2017/02/11 01:11:29, yzshen1 wrote:
> Formatting:
> 
> return interface_version >=
>     runOrClosePipeMsgParams.input.require_version.version;

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:56:
interfaceCtrlMsgs.QueryVersionResult({'version': interface_version});
On 2017/02/11 01:11:28, yzshen1 wrote:
> 4 spaces.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:66: payloadSize,
codec.kMessageIsResponse, requestID);
On 2017/02/11 01:11:29, yzshen1 wrote:
> 4 spaces

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:78: function
ControlMessageHandler(responder, interface_version) {
On 2017/02/11 01:11:28, yzshen1 wrote:
> Maybe we should remove responder from the constructor? Please also see my
> comment on line 88.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_handler.js:88:
ControlMessageHandler.prototype.acceptWithResponder = function(message) {
On 2017/02/11 01:11:28, yzshen1 wrote:
> The calling code pass a responder to this method, please see router.js.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
File mojo/public/js/lib/control_message_proxy.js (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:7: "mojo/public/js/validator",
On 2017/02/11 01:11:29, yzshen1 wrote:
> Alphabetically, please.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:9: ], function(interfaceCtrlMsgs,
validator, codec) {
On 2017/02/11 01:11:29, yzshen1 wrote:
> interfaceCtrlMsgs -> controlMessages?

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:31: if (!message.getName() &&
On 2017/02/11 01:11:29, yzshen1 wrote:
> Do we need this check at all?

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:33: throw new Error('Control message
name is not kRunMessageId');
On 2017/02/11 01:11:29, yzshen1 wrote:
> Please consider using double quotes.
> 
> And we need to validate the payload as well.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:37: function accept(message) {
On 2017/02/11 01:11:29, yzshen1 wrote:
> Could we give it a more descriptive name? We have too many methods named
accept.
> :)

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:59: // requestID is set to 0, but is
later
On 2017/02/11 01:11:29, yzshen1 wrote:
> Please fill this line to as close to 80 chars as possible.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:60: // properly set by Router's
acceptAndExpectResponse
On 2017/02/11 01:11:29, yzshen1 wrote:
> Usually in comments, we use xxx() to indicate function names and |xxx| to
> indicate variables.
> 
> Comments should be sentence with trailing "."

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:79: runResponseMsgParams) {
On 2017/02/11 01:11:29, yzshen1 wrote:
> 4 spaces.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:86:
interfaceCtrlMsgs.RunOrClosePipeMessageParams();
On 2017/02/11 01:11:29, yzshen1 wrote:
> 4 spaces.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/lib/cont...
mojo/public/js/lib/control_message_proxy.js:89:
interfaceCtrlMsgs.RequireVersion({'version': version});
On 2017/02/11 01:11:29, yzshen1 wrote:
> 4 spaces.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/router.js
File mojo/public/js/router.js (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/router.j...
mojo/public/js/router.js:33: this.controlMsgHandler_ = new
ControlMessageHandler(this,
On 2017/02/11 01:11:29, yzshen1 wrote:
> It is preferred to use full name: Msg -> Message. Unless it is something
> commonly used such as Url, Http.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/router.j...
mojo/public/js/router.js:34: interface_version);
On 2017/02/11 01:11:29, yzshen1 wrote:
> 4 spaces. Please see:
>
https://google.github.io/styleguide/javascriptguide.xml?showone=Code_formatti...

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/router.j...
mojo/public/js/router.js:118: controlMessageHandler.isControlMessage(message)) {
On 2017/02/11 01:11:29, yzshen1 wrote:
> if the message is a control message but we don't have a control message
handler,
> we should not pass this message to incomingReceiver_, instead we should
directly
> go to tear down the pipe.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/tests/in...
File mojo/public/js/tests/interface_ptr_unittest.js (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/tests/in...
mojo/public/js/tests/interface_ptr_unittest.js:181:
sampleInterfaces.IntegerAccessor,
On 2017/02/11 01:11:29, yzshen1 wrote:
> 4 spaces
> please see:
>
https://google.github.io/styleguide/javascriptguide.xml?showone=Code_formatti...

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/tests/in...
mojo/public/js/tests/interface_ptr_unittest.js:184: );
On 2017/02/11 01:11:30, yzshen1 wrote:
> ");" should not be on a separate line.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/tests/in...
mojo/public/js/tests/interface_ptr_unittest.js:197:
sampleInterfaces.IntegerAccessor,
On 2017/02/11 01:11:29, yzshen1 wrote:
> 4 spaces.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/js/tests/in...
mojo/public/js/tests/interface_ptr_unittest.js:235:
expect(integerAccessorImpl.integer).toBe(456);
On 2017/02/11 01:11:29, yzshen1 wrote:
> Because the call on line 234 is completed asynchronously, it won't change the
> value at this point. In order to really test that it is ignored, you should
move
> this line into the connection error handler on 229.

Done.

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/js_templates/module.amd.tmpl (right):

https://codereview.chromium.org/2676443005/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/module.amd.tmpl:6: {%-  if
module.path !=
"mojo/public/interfaces/bindings/interface_control_messages.mojom" %}
On 2017/02/11 01:11:30, yzshen1 wrote:
> nit: we don't have style guide for tmpl files, but please be consistent with
> surrounding code:
> 
> "{%- if" (only one space between "-" and "if")
> or "{%  if" (two spaces).
> 
> Nested blocks:
> 
> {%- if ... %}
> {%-   if ... %}
> {%    endif %}
> {%  endif %}

Done.

[commit-bot: I haz the power, 2017-02-13 22:16:21]

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

[commit-bot: I haz the power, 2017-02-13 22:16:21]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-02-13 23:44:23]

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
File mojo/public/js/lib/control_message_handler.js (right):

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
mojo/public/js/lib/control_message_handler.js:20: if (message.getName() !=
controlMessages.kRunMessageId) {
By the way, I found on the latest JS style guide
(https://google.github.io/styleguide/jsguide.html)
that

"""Braces are required for all control structures (i.e. if, else, for, do,
while, as well as any others), even if the body contains only a single
statement.

Exception: A simple if statement that can fit entirely on a single line with no
wrapping (and that doesn’t have an else) may be kept on a single line with no
braces when it improves readability. This is the only case in which a control
structure may omit braces and newlines."""

On
https://sites.google.com/a/chromium.org/dev/developers/web-development-style-...,
we have
"""Omit curly braces for single-line if statements."""

So my previous comment about "Whether to have {} around single-line body is not
enforced by the style guide." is incorrect with the latest style guide.

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
File mojo/public/js/lib/control_message_proxy.js (right):

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
mojo/public/js/lib/control_message_proxy.js:6: "mojo/public/js/codec",
alphabetically, please.

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
mojo/public/js/lib/control_message_proxy.js:54: * Accept the response message
from the receiver and decode.
Accept -> Accepts; decode -> decodes.
Because the (omitted) subject is "This method".

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
mojo/public/js/lib/control_message_proxy.js:55: * the message struct to
RunResponseMessageParams.
I guess this is part of the sentence of the previous line?
In that case, remove "." from prevoius line and fill it to as close to 80 chars
as possible.

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
mojo/public/js/lib/control_message_proxy.js:56: * .
Please remove "."

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/validat...
File mojo/public/js/validator.js (right):

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/validat...
mojo/public/js/validator.js:234:
Validator.prototype.validateMessageIsRequestWithoutResponse = function() {
Please remove redundant ones.

[wangjimmy, 2017-02-14 00:05:45]

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

[wangjimmy, 2017-02-14 00:06:06]

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
File mojo/public/js/lib/control_message_handler.js (right):

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
mojo/public/js/lib/control_message_handler.js:20: if (message.getName() !=
controlMessages.kRunMessageId) {
On 2017/02/13 23:44:23, yzshen1 wrote:
> By the way, I found on the latest JS style guide
> (https://google.github.io/styleguide/jsguide.html)
> that
> 
> """Braces are required for all control structures (i.e. if, else, for, do,
> while, as well as any others), even if the body contains only a single
> statement.
> 
> Exception: A simple if statement that can fit entirely on a single line with
no
> wrapping (and that doesn’t have an else) may be kept on a single line with no
> braces when it improves readability. This is the only case in which a control
> structure may omit braces and newlines."""
> 
> On
>
https://sites.google.com/a/chromium.org/dev/developers/web-development-style-...,
> we have
> """Omit curly braces for single-line if statements."""
> 
> So my previous comment about "Whether to have {} around single-line body is
not
> enforced by the style guide." is incorrect with the latest style guide.

Acknowledged.

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
File mojo/public/js/lib/control_message_proxy.js (right):

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
mojo/public/js/lib/control_message_proxy.js:6: "mojo/public/js/codec",
On 2017/02/13 23:44:23, yzshen1 wrote:
> alphabetically, please.

Done.

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
mojo/public/js/lib/control_message_proxy.js:54: * Accept the response message
from the receiver and decode.
On 2017/02/13 23:44:23, yzshen1 wrote:
> Accept -> Accepts; decode -> decodes.
> Because the (omitted) subject is "This method".
> 

Done.

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
mojo/public/js/lib/control_message_proxy.js:55: * the message struct to
RunResponseMessageParams.
On 2017/02/13 23:44:23, yzshen1 wrote:
> I guess this is part of the sentence of the previous line?
> In that case, remove "." from prevoius line and fill it to as close to 80
chars
> as possible.

Done.

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/lib/con...
mojo/public/js/lib/control_message_proxy.js:56: * .
On 2017/02/13 23:44:23, yzshen1 wrote:
> Please remove "."

Done.

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/validat...
File mojo/public/js/validator.js (right):

https://codereview.chromium.org/2676443005/diff/120001/mojo/public/js/validat...
mojo/public/js/validator.js:234:
Validator.prototype.validateMessageIsRequestWithoutResponse = function() {
On 2017/02/13 23:44:23, yzshen1 wrote:
> Please remove redundant ones.

Done.

[commit-bot: I haz the power, 2017-02-14 00:06:29]

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

[yzshen1, 2017-02-14 00:36:09]

LGTM

Please also add owners for files outside of mojo/
You could use "git cl owners" command to figure out who to add, or look up
OWNERS files.

[wangjimmy, 2017-02-14 00:59:40]

Description was changed from

==========
Add interface versioning for javascript.

Methods queryVersion and requireVersion are added to InterfacePtrController.
Support sending and handling control messages by creating control_message_proxy
and control_message_handler. control_message_proxy uses promises as opposed to
callbacks which is what our original cpp code does.

BUG=684879
R=yzshen@chromium.org
==========

to

==========
Add interface versioning for javascript.

Methods queryVersion and requireVersion are added to InterfacePtrController.
Support sending and handling control messages by creating control_message_proxy
and control_message_handler. control_message_proxy uses promises as opposed to
callbacks which is what our original cpp code does.

BUG=684879
R=yzshen@chromium.org
==========

[wangjimmy, 2017-02-14 00:59:40]

wangjimmy@chromium.org changed reviewers:
+ eugenebut@chromium.org

[wangjimmy, 2017-02-14 01:02:45]

Hi Eugune, I saw you were the owner for ios/web.
Would you mind reviewing my files related to that?

Thank you

[wangjimmy, 2017-02-14 01:18:13]

wangjimmy@chromium.org changed reviewers:
+ rockot@chromium.org

[wangjimmy, 2017-02-14 01:19:58]

Hi Ken,
Could you review
extensions/renderer/dispatcher.cc?

[wangjimmy, 2017-02-14 01:25:51]

wangjimmy@chromium.org changed reviewers:
+ jam@chromium.org

[wangjimmy, 2017-02-14 01:26:48]

wangjimmy@chromium.org changed reviewers:
- eugenebut@chromium.org, rockot@chromium.org, yzshen@chromium.org

[wangjimmy, 2017-02-14 01:26:49]

Hi John, do you mind reviewing:
content/renderer/mojo_context_state.cc
content/content_resources.grd
content/BUILD.gn

Thank you in advance.

[wangjimmy, 2017-02-14 01:28:28]

wangjimmy@chromium.org changed reviewers:
+ eugenebut@chromium.org, rockot@chromium.org

[Eugene But (OOO till 7-30), 2017-02-14 01:30:26]

ios lgtm

[commit-bot: I haz the power, 2017-02-14 02:15:09]

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

[commit-bot: I haz the power, 2017-02-14 02:15:10]

Dry run: This issue passed the CQ dry run.

[jam, 2017-02-14 17:51:22]

lgtm

[Ken Rockot(use gerrit already), 2017-02-14 17:55:16]

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

[Ken Rockot(use gerrit already), 2017-02-14 17:55:17]

lgtm

[wangjimmy, 2017-02-14 21:59:59]

The CQ bit was checked by wangjimmy@chromium.org

[commit-bot: I haz the power, 2017-02-14 22:00:35]

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-02-14 22:08:29]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487109599497400,
"parent_rev": "1ccc764ee77db82909842b22c04d11ce64de94dd", "commit_rev":
"43ca78b29ea7366bbbc35e28a272fb145771181f"}

[commit-bot: I haz the power, 2017-02-14 22:09:04]

Description was changed from

==========
Add interface versioning for javascript.

Methods queryVersion and requireVersion are added to InterfacePtrController.
Support sending and handling control messages by creating control_message_proxy
and control_message_handler. control_message_proxy uses promises as opposed to
callbacks which is what our original cpp code does.

BUG=684879
R=yzshen@chromium.org
==========

to

==========
Add interface versioning for javascript.

Methods queryVersion and requireVersion are added to InterfacePtrController.
Support sending and handling control messages by creating control_message_proxy
and control_message_handler. control_message_proxy uses promises as opposed to
callbacks which is what our original cpp code does.

BUG=684879
R=yzshen@chromium.org

Review-Url: https://codereview.chromium.org/2676443005
Cr-Commit-Position: refs/heads/master@{#450488}
Committed:
https://chromium.googlesource.com/chromium/src/+/43ca78b29ea7366bbbc35e28a272...
==========

[commit-bot: I haz the power, 2017-02-14 22:09:06]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/43ca78b29ea7366bbbc35e28a272...