Introduce InterfaceEndpointClient(IEC), InterfaceEndpointHandle and

Introduce InterfaceEndpointClient(IEC), InterfaceEndpointHandle and
PipeControlMessage Handler/Proxy. Move accept message and handling code from
Router to IEC. There is still only 1 interface on a message pipe, but this
work helps setup for associated interfaces.

BUG=695635
Review-Url: https://codereview.chromium.org/2744963002
Cr-Commit-Position: refs/heads/master@{#460817}
Committed: https://chromium.googlesource.com/chromium/src/+/087549eb773ef4b5fd43b5bd57f1a9ddfceeb486

[wangjimmy, 2017-03-11 09:40:23]

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

[commit-bot: I haz the power, 2017-03-11 09:40:36]

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-03-11 10:42:19]

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

[commit-bot: I haz the power, 2017-03-11 10:42:21]

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

[wangjimmy, 2017-03-17 02:15:14]

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

[commit-bot: I haz the power, 2017-03-17 02:15:41]

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-03-17 02:51:08]

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

[commit-bot: I haz the power, 2017-03-17 02:51:09]

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

[wangjimmy, 2017-03-17 22:13:15]

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

[commit-bot: I haz the power, 2017-03-17 22:14:12]

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-03-17 23:31:32]

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

[commit-bot: I haz the power, 2017-03-17 23:31:33]

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

[wangjimmy, 2017-03-20 17:56:22]

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

[wangjimmy, 2017-03-20 17:56:23]

[38036:1027:0317/212029.166458:35825003907766:ERROR:crw_web_controller.mm(2685)]
JavaScript error: Error: Load timeout for modules:
main,mojo/public/js/router,mojo/public/js/lib/interface_endpoint_handle

Hey Eugene, I'm getting this error in my try run "ios_simulator" when running
the WebUIMojoTest.MessageExchange test. Would you know why this is happening?

I suspect, I'm doing something incorrect with the
ios/web/webui/resources/timer.js module I added.

[Eugene But (OOO till 7-30), 2017-03-20 20:53:46]

>> main,mojo/public/js/router,mojo/public/js/lib/interface_endpoint_handle

>> Hey Eugene, I'm getting this error in my try run "ios_simulator" when running
>> the WebUIMojoTest.MessageExchange test. Would you know why this is happening?
Looks like there is something wrong with interface_endpoint_handle module. Which
leads to "router" load failure, and then to "main" load failure.

https://codereview.chromium.org/2744963002/diff/60001/ios/web/webui/crw_web_u...
File ios/web/webui/crw_web_ui_manager.mm (right):

https://codereview.chromium.org/2744963002/diff/60001/ios/web/webui/crw_web_u...
ios/web/webui/crw_web_ui_manager.mm:236: {mojo::kPipeControlMessagesMojom,
IDR_MOJO_PIPE_CONTROL_MESSAGES_MOJOM_JS},
This like exceeds allowed character width. Please run "git cl format".

https://codereview.chromium.org/2744963002/diff/60001/ios/web/webui/resources...
File ios/web/webui/resources/timer.js (right):

https://codereview.chromium.org/2744963002/diff/60001/ios/web/webui/resources...
ios/web/webui/resources/timer.js:10: // and setTimeout, setInterval is not
always available. When the Mojo JS
If this is a workaround, could you please add "TODO(crbug.com/###): Remove this
when..."

https://codereview.chromium.org/2744963002/diff/60001/ios/web/webui/resources...
ios/web/webui/resources/timer.js:15: * Logs a message to the console.
Please update the comment to reflect what this function is doing.

https://codereview.chromium.org/2744963002/diff/60001/ios/web/webui/resources...
ios/web/webui/resources/timer.js:22: function createRepeating(delay, callback) {
Please add a comment

[wangjimmy, 2017-03-21 03:52:59]

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

[commit-bot: I haz the power, 2017-03-21 03:53:19]

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

[wangjimmy, 2017-03-21 03:55:07]

The CQ bit was unchecked by wangjimmy@chromium.org

[wangjimmy, 2017-03-21 04:35:34]

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

[commit-bot: I haz the power, 2017-03-21 04:35:50]

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-03-21 05:35:35]

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

[commit-bot: I haz the power, 2017-03-21 05:35:36]

Dry run: This issue passed the CQ dry run.

[wangjimmy, 2017-03-21 16:53:37]

Hey Eugune, PTAL at ios/web/webui

Also I added the "TODO(crbug.com/703380): When the Mojo JS bindings move away
from this module could be removed" to both timer.js and console.js

https://codereview.chromium.org/2744963002/diff/60001/ios/web/webui/crw_web_u...
File ios/web/webui/crw_web_ui_manager.mm (right):

https://codereview.chromium.org/2744963002/diff/60001/ios/web/webui/crw_web_u...
ios/web/webui/crw_web_ui_manager.mm:236: {mojo::kPipeControlMessagesMojom,
IDR_MOJO_PIPE_CONTROL_MESSAGES_MOJOM_JS},
On 2017/03/20 20:53:46, Eugene But wrote:
> This like exceeds allowed character width. Please run "git cl format".

Done.

https://codereview.chromium.org/2744963002/diff/60001/ios/web/webui/resources...
File ios/web/webui/resources/timer.js (right):

https://codereview.chromium.org/2744963002/diff/60001/ios/web/webui/resources...
ios/web/webui/resources/timer.js:10: // and setTimeout, setInterval is not
always available. When the Mojo JS
On 2017/03/20 20:53:46, Eugene But wrote:
> If this is a workaround, could you please add "TODO(crbug.com/###): Remove
this
> when..."

Done.

https://codereview.chromium.org/2744963002/diff/60001/ios/web/webui/resources...
ios/web/webui/resources/timer.js:15: * Logs a message to the console.
On 2017/03/20 20:53:46, Eugene But wrote:
> Please update the comment to reflect what this function is doing.

Done.

https://codereview.chromium.org/2744963002/diff/60001/ios/web/webui/resources...
ios/web/webui/resources/timer.js:22: function createRepeating(delay, callback) {
On 2017/03/20 20:53:46, Eugene But wrote:
> Please add a comment

Done.

[Eugene But (OOO till 7-30), 2017-03-21 17:12:17]

ios lgtm

[wangjimmy, 2017-03-21 18:36:36]

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

[commit-bot: I haz the power, 2017-03-21 18:37:18]

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

[wangjimmy, 2017-03-21 19:56:31]

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

[wangjimmy, 2017-03-21 19:56:31]

Hey Ken, PTAL at extensions/

[wangjimmy, 2017-03-21 19:57:49]

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

[wangjimmy, 2017-03-21 19:57:50]

Hi John, PTAL at content/. Thanks!

[commit-bot: I haz the power, 2017-03-21 20:01:15]

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

[commit-bot: I haz the power, 2017-03-21 20:01:16]

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_...)

[wangjimmy, 2017-03-21 20:31:57]

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

[wangjimmy, 2017-03-21 20:31:59]

Hi Yuzhu, PTAL at mojo/ and
third_party/WebKit/LayoutTests/mojo/connection.html
third_party/WebKit/LayoutTests/mojo/interface_ptr.html

[Ken Rockot(use gerrit already), 2017-03-21 23:19:06]

extensions lgtm

[jam, 2017-03-22 00:04:29]

lgtm

[wangjimmy, 2017-03-22 05:50:42]

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

[commit-bot: I haz the power, 2017-03-22 05:51:29]

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-03-22 07:22:20]

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

[commit-bot: I haz the power, 2017-03-22 07:22:23]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-03-24 21:20:13]

First part of comments.

Sorry for the delay!

https://codereview.chromium.org/2744963002/diff/180001/ios/web/webui/resource...
File ios/web/webui/resources/timer.js (right):

https://codereview.chromium.org/2744963002/diff/180001/ios/web/webui/resource...
ios/web/webui/resources/timer.js:10: // and createOneShot, createRepeating is
not always available.
is -> are?

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/binding...
File mojo/public/js/bindings.js (right):

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/binding...
mojo/public/js/bindings.js:37: this.controlMessageProxy_ = null;
Please update this list to include all members and remove unused ones.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/binding...
mojo/public/js/bindings.js:72: if (this.interfaceEndpointClient_) {
It seems nice to move this block above line 62. Although it probably doesn't
matter, conceptually it makes sense to destroy IEH before the router.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/binding...
mojo/public/js/bindings.js:73: this.interfaceEndpointClient_.passHandle();
Here we need to close the handle. You could use closeWithReason() with a
undefined |reason|, or expose a close() method?

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/binding...
mojo/public/js/bindings.js:178: this.stub_ = null;
Please update this list.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/binding...
mojo/public/js/bindings.js:219: this.interfaceEndpointClient_.passHandle();
The handle needs to be closed. Please see my comment for
InterfacePtrController.prototype.reset.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/constan...
File mojo/public/js/constants.h (right):

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/constan...
mojo/public/js/constants.h:22: extern const char
kPipeControlMessageHandlerModuleName[];
nit: please order these names alphabetically (and also do that in the .cc file).

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
File mojo/public/js/lib/interface_endpoint_client.js (right):

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:13: ], function(console, codec,
controlMessageHandler, controlMessageProxy,
Style nit: For function definitions (as opposed to function calls), when
parameters cannot fit on one line, typically we put each on a separate line.

function(param1,
         param2,
         ...) {
}

(here and elsewhere in this patch.)

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:29: if
(interfaceEndpointHandle.pendingAssociation()) {
Please move this block to the end, after declaration of all member variables.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:46: this.connectionErrorHandler_
= undefined;
I noticed that some fields are inited as "undefined" but some are "null". Why is
the difference?

I usually use "null". They are defined just not yet refer to a valid object.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:49:
InterfaceEndpointClient.prototype.initControllerIfNecessary = function() {
private functions should end with trailing underscore.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:61:
InterfaceEndpointHandle.AssociationEvent.ASSOCIATED) {
Indent should be 4 spaces.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:64:
InterfaceEndpointHandle.AssociationEvent.PEER_CLOSED_BEFORE_ASSOCIATION) {
ditto

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:102: {
{ should be on the same line as the parameter list.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:127:
InterfaceEndpointClient.prototype.reject = function(message) {
How is this used?

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:132: payloadValidators) {
4-space indent.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:141: function(handler) {
ditto

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:146: {
{ should be on the same line as parameter list.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:156: else {
This should be on the same line as the previous "}".

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:194:
this.controlMessageHandler_) {
wrong indent.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
File mojo/public/js/lib/interface_endpoint_handle.js (right):

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:19: interfaceId =
types.kInvalidInterfaceId
trailing ";"

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:40: State.prototype.close =
function(disconnectReason) {
Please also reset associationEventHandler_, interfaceId, pendingAssociation,
peerState as needed.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:52:
this.associationEventHandler_ = undefined;
Although it shouldn't matter in this particular case, it is generally a good
practice to assign this handler to a local variable and set it to undefined, and
then run the handler.

That way if the handler does something resulting in a new
this.associationEventHandler_ to be set. It won't be reset to undefined
afterwards unintentionally.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:56: timer.createOneShot(0,
run.bind(this));
At the time when run() is run, this.asssociationEventHandler_ may have been
reset.

Therefore, the "if (this.associationEventHandler_)" check should be done inside
run() instead.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:62:
!types.isValidInterfaceId(this.interfaceId)) || !handler) {
If handler is null, you need to reset this.associationEventHandler_.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:85:
this.runAssociationEventHandler(AssociationEvent.ASSOCIATED);
We have to run the handler immediately instead of delay it with a timer in this
case. The reason:

FooAssociatedPtr foo_ptr;
FooAssociatedRequest request = makeRequest(foo_ptr);

another_ptr.sendFooRequest(request);

// At this point, we must have already notified foo_ptr about the association,
otherwise you cannot use foo_ptr immediately.
foo_ptr.callMethod();

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:96:
this.runAssociationEventHandler(
Run the handler immediately instead of delaying it with a timer. In IEC you will
delay the notification to the user with a timer already, so no need to do it
twice.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/tests/v...
File mojo/public/js/tests/validation_unittest.js (right):

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/tests/v...
mojo/public/js/tests/validation_unittest.js:284: observer.reset();
nit: you could move this line to the end of the for-loop.

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

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/validat...
mojo/public/js/validator.js:42: gValidationErrorObserver = this;
This line is redundant (please see line 61).

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/validat...
mojo/public/js/validator.js:69: if (gValidationErrorObserver) {
Could we do "return !!gValidationErrorObserver"?

[wangjimmy, 2017-03-25 04:42:51]

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

[commit-bot: I haz the power, 2017-03-25 04:43:01]

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-03-25 05:50:16]

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

[commit-bot: I haz the power, 2017-03-25 05:50:17]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[wangjimmy, 2017-03-27 16:51:28]

https://codereview.chromium.org/2744963002/diff/180001/ios/web/webui/resource...
File ios/web/webui/resources/timer.js (right):

https://codereview.chromium.org/2744963002/diff/180001/ios/web/webui/resource...
ios/web/webui/resources/timer.js:10: // and createOneShot, createRepeating is
not always available.
On 2017/03/24 21:20:12, yzshen1 wrote:
> is -> are?

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/binding...
File mojo/public/js/bindings.js (right):

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/binding...
mojo/public/js/bindings.js:37: this.controlMessageProxy_ = null;
On 2017/03/24 21:20:12, yzshen1 wrote:
> Please update this list to include all members and remove unused ones.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/binding...
mojo/public/js/bindings.js:72: if (this.interfaceEndpointClient_) {
On 2017/03/24 21:20:12, yzshen1 wrote:
> It seems nice to move this block above line 62. Although it probably doesn't
> matter, conceptually it makes sense to destroy IEH before the router.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/binding...
mojo/public/js/bindings.js:73: this.interfaceEndpointClient_.passHandle();
On 2017/03/24 21:20:12, yzshen1 wrote:
> Here we need to close the handle. You could use closeWithReason() with a
> undefined |reason|, or expose a close() method?

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/binding...
mojo/public/js/bindings.js:178: this.stub_ = null;
On 2017/03/24 21:20:12, yzshen1 wrote:
> Please update this list.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/binding...
mojo/public/js/bindings.js:219: this.interfaceEndpointClient_.passHandle();
On 2017/03/24 21:20:12, yzshen1 wrote:
> The handle needs to be closed. Please see my comment for
> InterfacePtrController.prototype.reset.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/constan...
File mojo/public/js/constants.h (right):

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/constan...
mojo/public/js/constants.h:22: extern const char
kPipeControlMessageHandlerModuleName[];
On 2017/03/24 21:20:12, yzshen1 wrote:
> nit: please order these names alphabetically (and also do that in the .cc
file).

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
File mojo/public/js/lib/interface_endpoint_client.js (right):

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:13: ], function(console, codec,
controlMessageHandler, controlMessageProxy,
On 2017/03/24 21:20:12, yzshen1 wrote:
> Style nit: For function definitions (as opposed to function calls), when
> parameters cannot fit on one line, typically we put each on a separate line.
> 
> function(param1,
>          param2,
>          ...) {
> }
> 
> (here and elsewhere in this patch.)

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:29: if
(interfaceEndpointHandle.pendingAssociation()) {
On 2017/03/24 21:20:13, yzshen1 wrote:
> Please move this block to the end, after declaration of all member variables.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:46: this.connectionErrorHandler_
= undefined;
On 2017/03/24 21:20:13, yzshen1 wrote:
> I noticed that some fields are inited as "undefined" but some are "null". Why
is
> the difference?
> 
> I usually use "null". They are defined just not yet refer to a valid object.

Done. I was trying to use undefined for callbacks. But using null for everything
would be more consistent, thanks!

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:49:
InterfaceEndpointClient.prototype.initControllerIfNecessary = function() {
On 2017/03/24 21:20:12, yzshen1 wrote:
> private functions should end with trailing underscore.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:61:
InterfaceEndpointHandle.AssociationEvent.ASSOCIATED) {
On 2017/03/24 21:20:12, yzshen1 wrote:
> Indent should be 4 spaces.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:64:
InterfaceEndpointHandle.AssociationEvent.PEER_CLOSED_BEFORE_ASSOCIATION) {
On 2017/03/24 21:20:12, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:102: {
On 2017/03/24 21:20:12, yzshen1 wrote:
> { should be on the same line as the parameter list.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:127:
InterfaceEndpointClient.prototype.reject = function(message) {
On 2017/03/24 21:20:13, yzshen1 wrote:
> How is this used?

This method is never used.
Looking at the git blame, this TODO was added as part of
https://chromium.googlesource.com/chromium/src/+/5243bce526df42358983d4068647...

I don't see a way to transmit errors over a Connection in connector.cc or
multiplex_router.cc either so I think it's safe to remove this.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:132: payloadValidators) {
On 2017/03/24 21:20:12, yzshen1 wrote:
> 4-space indent.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:141: function(handler) {
On 2017/03/24 21:20:12, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:146: {
On 2017/03/24 21:20:12, yzshen1 wrote:
> { should be on the same line as parameter list.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:156: else {
On 2017/03/24 21:20:12, yzshen1 wrote:
> This should be on the same line as the previous "}".

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:194:
this.controlMessageHandler_) {
On 2017/03/24 21:20:13, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
File mojo/public/js/lib/interface_endpoint_handle.js (right):

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:19: interfaceId =
types.kInvalidInterfaceId
On 2017/03/24 21:20:13, yzshen1 wrote:
> trailing ";"

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:40: State.prototype.close =
function(disconnectReason) {
On 2017/03/24 21:20:13, yzshen1 wrote:
> Please also reset associationEventHandler_, interfaceId, pendingAssociation,
> peerState as needed.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:52:
this.associationEventHandler_ = undefined;
On 2017/03/24 21:20:13, yzshen1 wrote:
> Although it shouldn't matter in this particular case, it is generally a good
> practice to assign this handler to a local variable and set it to undefined,
and
> then run the handler.
> 
> That way if the handler does something resulting in a new
> this.associationEventHandler_ to be set. It won't be reset to undefined
> afterwards unintentionally.

Done, changed it. Since you're right a callback could set the handler. Doesn't
happen in this case, but still good practice to follow. Thanks!

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:56: timer.createOneShot(0,
run.bind(this));
On 2017/03/24 21:20:13, yzshen1 wrote:
> At the time when run() is run, this.asssociationEventHandler_ may have been
> reset.
> 
> Therefore, the "if (this.associationEventHandler_)" check should be done
inside
> run() instead.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:62:
!types.isValidInterfaceId(this.interfaceId)) || !handler) {
On 2017/03/24 21:20:13, yzshen1 wrote:
> If handler is null, you need to reset this.associationEventHandler_.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:85:
this.runAssociationEventHandler(AssociationEvent.ASSOCIATED);
On 2017/03/24 21:20:13, yzshen1 wrote:
> We have to run the handler immediately instead of delay it with a timer in
this
> case. The reason:
> 
> FooAssociatedPtr foo_ptr;
> FooAssociatedRequest request = makeRequest(foo_ptr);
> 
> another_ptr.sendFooRequest(request);
> 
> // At this point, we must have already notified foo_ptr about the association,
> otherwise you cannot use foo_ptr immediately.
> foo_ptr.callMethod();
> 

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_handle.js:96:
this.runAssociationEventHandler(
On 2017/03/24 21:20:13, yzshen1 wrote:
> Run the handler immediately instead of delaying it with a timer. In IEC you
will
> delay the notification to the user with a timer already, so no need to do it
> twice.

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/tests/v...
File mojo/public/js/tests/validation_unittest.js (right):

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/tests/v...
mojo/public/js/tests/validation_unittest.js:284: observer.reset();
On 2017/03/24 21:20:13, yzshen1 wrote:
> nit: you could move this line to the end of the for-loop.

Done.

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

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/validat...
mojo/public/js/validator.js:42: gValidationErrorObserver = this;
On 2017/03/24 21:20:13, yzshen1 wrote:
> This line is redundant (please see line 61).

Done.

https://codereview.chromium.org/2744963002/diff/180001/mojo/public/js/validat...
mojo/public/js/validator.js:69: if (gValidationErrorObserver) {
On 2017/03/24 21:20:13, yzshen1 wrote:
> Could we do "return !!gValidationErrorObserver"?

Done. I decided to do "return Boolean(gValidationErrorObserver);" so it's more
explicit.

[yzshen1, 2017-03-28 00:46:00]

A few more comments.
I think it mostly looks good.


Thanks!

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/binding...
File mojo/public/js/bindings.js (right):

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/binding...
mojo/public/js/bindings.js:39: this.controlMessageProxy_ = null;
I think this one is not used anymore?

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/binding...
mojo/public/js/bindings.js:65: this.interfaceEndpointClient_.closeWithReason();
How about we rename the method to close(), which can optionally take a reason?

It is a little weird to call closeWithReason() without any param.

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/lib/int...
File mojo/public/js/lib/interface_endpoint_client.js (right):

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:108: message){
nit: one space between ) and {

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:195:
this.controlMessageHandler_) {
The indent is still incorrect.

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/lib/pip...
File mojo/public/js/lib/pipe_control_message_proxy.js (right):

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/lib/pip...
mojo/public/js/lib/pipe_control_message_proxy.js:29: this.receiver = receiver;
if this.receiver is not supposed to be public, it needs to have a trailing "_".

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

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/router....
mojo/public/js/router.js:42: this.eventSignalled = false;
This is not needed.

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/router....
mojo/public/js/router.js:50: function AssociatedGroupController() {
Because we don't have multiple implementation of AssociatedGroupController, we
could merge it with Router.

WDYT?

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/router....
mojo/public/js/router.js:90: } else {
Thinking about it some more, I think maybe it makes sense to add those
CHECK/DCHECK from the C++ code using a home-made check function:

function check(condition, output) {
  if (!condition)
    throw new Error(output);
}

This code is pretty complex so it would be much easier to debug if we have these
checks to ensure state consistency.

WDYT?

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/router....
mojo/public/js/router.js:102: function Router(handle,
setInterfaceIdNamespaceBit,connectorFactory) {
Do we still need this "connectorFactory" param? (If yes, please add one space
between "," and "connectorFactory")

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/router....
mojo/public/js/router.js:184: var endpoint = this.endpoints_.get(interfaceId);
If the endpoint doesn't exist, we need to insert it. That could happen if the
remote side sends us this control message before sending the corresponding
associated endpoint handle.

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/router....
mojo/public/js/router.js:189: if (!endpoint.peerClosed && endpoint.client) {
This is incorrect, even if endpoint.client is not currently set, we still need
to do updateEndpointStateMayRemove

[wangjimmy, 2017-03-28 05:23:36]

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

[commit-bot: I haz the power, 2017-03-28 05:23:57]

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-03-28 06:32:23]

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

[commit-bot: I haz the power, 2017-03-28 06:32:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wangjimmy, 2017-03-29 02:56:53]

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

[commit-bot: I haz the power, 2017-03-29 02:57:43]

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-03-29 04:59:13]

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

[commit-bot: I haz the power, 2017-03-29 04:59:14]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[wangjimmy, 2017-03-29 17:01:19]

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/binding...
File mojo/public/js/bindings.js (right):

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/binding...
mojo/public/js/bindings.js:39: this.controlMessageProxy_ = null;
On 2017/03/28 00:45:59, yzshen1 wrote:
> I think this one is not used anymore?

Done.

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/binding...
mojo/public/js/bindings.js:65: this.interfaceEndpointClient_.closeWithReason();
On 2017/03/28 00:45:59, yzshen1 wrote:
> How about we rename the method to close(), which can optionally take a reason?
> 
> It is a little weird to call closeWithReason() without any param.

Done. I agreed. Renamed to close

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/lib/int...
File mojo/public/js/lib/interface_endpoint_client.js (right):

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:108: message){
On 2017/03/28 00:46:00, yzshen1 wrote:
> nit: one space between ) and {

Done.

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/lib/int...
mojo/public/js/lib/interface_endpoint_client.js:195:
this.controlMessageHandler_) {
On 2017/03/28 00:46:00, yzshen1 wrote:
> The indent is still incorrect.

Done.

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/lib/pip...
File mojo/public/js/lib/pipe_control_message_proxy.js (right):

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/lib/pip...
mojo/public/js/lib/pipe_control_message_proxy.js:29: this.receiver = receiver;
On 2017/03/28 00:46:00, yzshen1 wrote:
> if this.receiver is not supposed to be public, it needs to have a trailing
"_".

Done.

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

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/router....
mojo/public/js/router.js:42: this.eventSignalled = false;
On 2017/03/28 00:46:00, yzshen1 wrote:
> This is not needed.

Done.

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/router....
mojo/public/js/router.js:50: function AssociatedGroupController() {
On 2017/03/28 00:46:00, yzshen1 wrote:
> Because we don't have multiple implementation of AssociatedGroupController, we
> could merge it with Router.
> 
> WDYT?

Done. Merged it with Router.

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/router....
mojo/public/js/router.js:90: } else {
On 2017/03/28 00:46:00, yzshen1 wrote:
> Thinking about it some more, I think maybe it makes sense to add those
> CHECK/DCHECK from the C++ code using a home-made check function:
> 
> function check(condition, output) {
>   if (!condition)
>     throw new Error(output);
> }
> 
> This code is pretty complex so it would be much easier to debug if we have
these
> checks to ensure state consistency.
> 
> WDYT?

Done. I agree, it would help with debugging. Added check for every relevant
DCHECK/CHECK from multiplex_router.cc

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/router....
mojo/public/js/router.js:102: function Router(handle,
setInterfaceIdNamespaceBit,connectorFactory) {
On 2017/03/28 00:46:00, yzshen1 wrote:
> Do we still need this "connectorFactory" param? (If yes, please add one space
> between "," and "connectorFactory")

Done. Removed it. We don't use this anywhere. It was used for a TestRouter, but
that was removed in
https://chromium.googlesource.com/chromium/src/+/4cbc282a724bad6fb6dec8b3ed0f...

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/router....
mojo/public/js/router.js:184: var endpoint = this.endpoints_.get(interfaceId);
On 2017/03/28 00:46:00, yzshen1 wrote:
> If the endpoint doesn't exist, we need to insert it. That could happen if the
> remote side sends us this control message before sending the corresponding
> associated endpoint handle.

Done.

https://codereview.chromium.org/2744963002/diff/200001/mojo/public/js/router....
mojo/public/js/router.js:189: if (!endpoint.peerClosed && endpoint.client) {
On 2017/03/28 00:46:00, yzshen1 wrote:
> This is incorrect, even if endpoint.client is not currently set, we still need
> to do updateEndpointStateMayRemove

Done.

[yzshen1, 2017-03-29 22:05:35]

LGTM

Thanks!

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

https://codereview.chromium.org/2744963002/diff/240001/mojo/public/js/router....
mojo/public/js/router.js:36: throw new Error(output);
[Just to double check] It seems the callers never pass a valid output. What will
be displayed on screen when a check fails? Is it sufficient to figure out the
issue?

https://codereview.chromium.org/2744963002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/vibration/vibration-iframe-expected.txt
(right):

https://codereview.chromium.org/2744963002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/vibration/vibration-iframe-expected.txt:1:
CONSOLE ERROR: line 300: Uncaught TypeError: Cannot read property 'then' of
undefined
This seems unfortunate. :/ Does this break everytime we change our bindings
code?

[wangjimmy, 2017-03-29 23:35:26]

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

[commit-bot: I haz the power, 2017-03-29 23:36:41]

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

[wangjimmy, 2017-03-30 00:02:22]

>
https://codereview.chromium.org/2744963002/diff/240001/mojo/public/js/router....
> mojo/public/js/router.js:36: throw new Error(output);
> [Just to double check] It seems the callers never pass a valid output. What
will
> be displayed on screen when a check fails? Is it sufficient to figure out the
> issue?

New patch adds this to help better debug.

   function check(condition, output) {
     if (!condition) {
-      throw new Error(output);
+      // testharness.js does not rethrow errors so the error stack needs to be
+      // included as a string in the error we throw for debugging layout tests.
+      throw new Error((new Error()).stack);
     }
   }


>
https://codereview.chromium.org/2744963002/diff/240001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/vibration/vibration-iframe-expected.txt:1:
> CONSOLE ERROR: line 300: Uncaught TypeError: Cannot read property 'then' of
> undefined
> This seems unfortunate. :/ Does this break everytime we change our bindings
> code?

This does break every time we change our tmpl code for mojom JS generator. I
brought the issue up to reilly. There isn't a current plan to improve the way
layout tests work.
If this becomes a more frequent problem, we could bring it up again.

[commit-bot: I haz the power, 2017-03-30 01:39:12]

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

[commit-bot: I haz the power, 2017-03-30 01:39:13]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[wangjimmy, 2017-03-30 17:41:18]

The CQ bit was checked by wangjimmy@chromium.org

[wangjimmy, 2017-03-30 17:41:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, rockot@chromium.org, jam@chromium.org,
yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2744963002/#ps260001
(title: "Throw the error with the string being the stack trace needed to debug
layouts which don't output an…")

[commit-bot: I haz the power, 2017-03-30 17:41:55]

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-03-30 17:50:01]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1490895678866790,
"parent_rev": "ea504d4d9555021596c5ad61740cbe5507d7a680", "commit_rev":
"087549eb773ef4b5fd43b5bd57f1a9ddfceeb486"}

[commit-bot: I haz the power, 2017-03-30 17:50:59]

Description was changed from

==========
Introduce InterfaceEndpointClient(IEC), InterfaceEndpointHandle and
PipeControlMessage Handler/Proxy. Move accept message and handling code from
Router to IEC. There is still only 1 interface on a message pipe, but this
work helps setup for associated interfaces.

BUG=695635
==========

to

==========
Introduce InterfaceEndpointClient(IEC), InterfaceEndpointHandle and
PipeControlMessage Handler/Proxy. Move accept message and handling code from
Router to IEC. There is still only 1 interface on a message pipe, but this
work helps setup for associated interfaces.

BUG=695635

Review-Url: https://codereview.chromium.org/2744963002
Cr-Commit-Position: refs/heads/master@{#460817}
Committed:
https://chromium.googlesource.com/chromium/src/+/087549eb773ef4b5fd43b5bd57f1...
==========

[commit-bot: I haz the power, 2017-03-30 17:51:01]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/087549eb773ef4b5fd43b5bd57f1...

[kolos1, 2017-04-03 10:56:10]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/2788403002/ by kolos@chromium.org.

The reason for reverting is: This CL makes
BluetoothInternalsTest.Startup_BluetoothInternals flaky. See FindIt report
https://findit-for-me.appspot.com/waterfall/failure?url=https%3A%2F%2Fbuild.c...

Also check details in the bug.
BUG=707725.

[kolos1, 2017-04-03 11:06:53]

On 2017/04/03 10:56:10, kolos1 wrote:
> A revert of this CL (patchset #14 id:260001) has been created in
> https://codereview.chromium.org/2788403002/ by mailto:kolos@chromium.org.
> 
> The reason for reverting is: This CL makes
> BluetoothInternalsTest.Startup_BluetoothInternals flaky. See FindIt report
>
https://findit-for-me.appspot.com/waterfall/failure?url=https%3A%2F%2Fbuild.c...
> 
> Also check details in the bug.
> BUG=707725.

I requested reverting, but realized that this CL doesn't touch any bluetooth
stuff. So, the test should be disable. I canceled reverting.

[agrieve, 2017-04-03 14:37:27]

On 2017/04/03 11:06:53, kolos1 wrote:
> On 2017/04/03 10:56:10, kolos1 wrote:
> > A revert of this CL (patchset #14 id:260001) has been created in
> > https://codereview.chromium.org/2788403002/ by mailto:kolos@chromium.org.
> > 
> > The reason for reverting is: This CL makes
> > BluetoothInternalsTest.Startup_BluetoothInternals flaky. See FindIt report
> >
>
https://findit-for-me.appspot.com/waterfall/failure?url=https%3A%2F%2Fbuild.c...
> > 
> > Also check details in the bug.
> > BUG=707725.
> 
> I requested reverting, but realized that this CL doesn't touch any bluetooth
> stuff. So, the test should be disable. I canceled reverting.

This increased android libchrome.so by 20kb. Made a note this on existing mojo
size bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=597125