Add associated binding set. Add associated_binding.html layout test. (issue 2844133003 by wangjimmy@chromium.org)

[wang...@chromium.org]

Reviewers: yzshen1
CL: https://codereview.chromium.org/2844133003/

Message:
Hi Yuzhu, PTAL. Thank you

Description:
Add associated binding set. Add associated_binding.html layout test.

BUG=695635

Affected files (+210, -5 lines):
M mojo/public/js/associated_bindings.js
M mojo/public/js/bindings.js
A third_party/WebKit/LayoutTests/mojo/associated_binding.html

[yzs...@chromium.org]

https://codereview.chromium.org/2844133003/diff/1/mojo/public/js/associated_bindings.js
File mojo/public/js/associated_bindings.js (right):

https://codereview.chromium.org/2844133003/diff/1/mojo/public/js/associated_bindings.js#newcode237
mojo/public/js/associated_bindings.js:237: function
AssociatedBindingSetEntry(associatedBindingSet, interfaceType, impl,
For function definition, please either fit all parameters on a single
line (if possible), or put each on a separate line and align them.
(There is no such requirement for function calls.)

https://codereview.chromium.org/2844133003/diff/1/mojo/public/js/associated_bindings.js#newcode263
mojo/public/js/associated_bindings.js:263:
AssociatedBindingSet.prototype.addBinding = function(impl,
Could we design BindingSetBase/BindingSetEntry to be able to handle
associated/non-associated case. And then make
BindingSet/AssociatedBindingSet inherit BindingSetBase?

It seems more robust than overriding some methods from BindingSet.

https://codereview.chromium.org/2844133003/diff/1/third_party/WebKit/LayoutTests/mojo/associated_binding.html
File third_party/WebKit/LayoutTests/mojo/associated_binding.html
(right):

https://codereview.chromium.org/2844133003/diff/1/third_party/WebKit/LayoutTests/mojo/associated_binding.html#newcode86
third_party/WebKit/LayoutTests/mojo/associated_binding.html:86:
setTimeout(integerSenderConnectionBinding.close.bind(
Would you please explain this line?

https://codereview.chromium.org/2844133003/diff/1/third_party/WebKit/LayoutTests/mojo/associated_binding.html#newcode101
third_party/WebKit/LayoutTests/mojo/associated_binding.html:101: var
integerSenderPtrInfo0 =
Please use a for loop to do the work, instead of duplicating the code.

https://codereview.chromium.org/2844133003/

[wang...@chromium.org]

https://codereview.chromium.org/2844133003/diff/1/mojo/public/js/associated_bindings.js
File mojo/public/js/associated_bindings.js (right):

https://codereview.chromium.org/2844133003/diff/1/mojo/public/js/associated_bindings.js#newcode237
mojo/public/js/associated_bindings.js:237: function
AssociatedBindingSetEntry(associatedBindingSet, interfaceType, impl,

On 2017/04/27 17:28:38, yzshen1 wrote:
> For function definition, please either fit all parameters on a single
line (if
> possible), or put each on a separate line and align them.
> (There is no such requirement for function calls.)

Done.

https://codereview.chromium.org/2844133003/diff/1/mojo/public/js/associated_bindings.js#newcode263
mojo/public/js/associated_bindings.js:263:
AssociatedBindingSet.prototype.addBinding = function(impl,

On 2017/04/27 17:28:38, yzshen1 wrote:
> Could we design BindingSetBase/BindingSetEntry to be able to handle
> associated/non-associated case. And then make
BindingSet/AssociatedBindingSet
> inherit BindingSetBase?
>
> It seems more robust than overriding some methods from BindingSet.

Done.

https://codereview.chromium.org/2844133003/diff/1/third_party/WebKit/LayoutTests/mojo/associated_binding.html
File third_party/WebKit/LayoutTests/mojo/associated_binding.html
(right):

https://codereview.chromium.org/2844133003/diff/1/third_party/WebKit/LayoutTests/mojo/associated_binding.html#newcode86
third_party/WebKit/LayoutTests/mojo/associated_binding.html:86:
setTimeout(integerSenderConnectionBinding.close.bind(

On 2017/04/27 17:28:38, yzshen1 wrote:
> Would you please explain this line?

Fixed.

https://codereview.chromium.org/2844133003/diff/1/third_party/WebKit/LayoutTests/mojo/associated_binding.html#newcode101
third_party/WebKit/LayoutTests/mojo/associated_binding.html:101: var
integerSenderPtrInfo0 =

On 2017/04/27 17:28:38, yzshen1 wrote:
> Please use a for loop to do the work, instead of duplicating the code.

Done.

https://codereview.chromium.org/2844133003/

[yzs...@chromium.org]

LGTM with one comment.


https://codereview.chromium.org/2844133003/diff/20001/mojo/public/js/associated_bindings.js
File mojo/public/js/associated_bindings.js (right):

https://codereview.chromium.org/2844133003/diff/20001/mojo/public/js/associated_bindings.js#newcode242
mojo/public/js/associated_bindings.js:242: this.bindingType =
AssociatedBinding;
Please make this a private member.

https://codereview.chromium.org/2844133003/

[wang...@chromium.org]

https://codereview.chromium.org/2844133003/diff/20001/mojo/public/js/associated_bindings.js
File mojo/public/js/associated_bindings.js (right):

https://codereview.chromium.org/2844133003/diff/20001/mojo/public/js/associated_bindings.js#newcode242
mojo/public/js/associated_bindings.js:242: this.bindingType =
AssociatedBinding;

On 2017/04/27 23:46:00, yzshen1 wrote:
> Please make this a private member.

Done.

https://codereview.chromium.org/2844133003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2844133003/80001

https://codereview.chromium.org/2844133003/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/46cdc9add2a99d8e664df0e8ed4e55df530c48d1

https://codereview.chromium.org/2844133003/