Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844133003:
Add associated binding set. Add associated_binding.html layout test. (Closed)
|
Created:
3 years, 8 months ago by wangjimmy Modified:
3 years, 7 months ago Reviewers:
yzshen1 CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, blink-reviews, darin (slow to review) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd associated binding set. Add associated_binding.html layout test.
BUG=695635
Review-Url: https://codereview.chromium.org/2844133003
Cr-Commit-Position: refs/heads/master@{#468288}
Committed: https://chromium.googlesource.com/chromium/src/+/46cdc9add2a99d8e664df0e8ed4e55df530c48d1
Patch Set 1 #
Total comments: 8
Patch Set 2 : Simplify binding and associated bindings code. Fix associated_binding.html test. #
Total comments: 2
Patch Set 3 : Make bindingType a private member #Patch Set 4 : Revert "Make bindingType a private member" #Patch Set 5 : Make bindingType private and passed it into BindingSetEntry #
Messages
Total messages: 30 (23 generated)
The CQ bit was checked by wangjimmy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wangjimmy@chromium.org changed reviewers: + yzshen@chromium.org
Hi Yuzhu, PTAL. Thank you
https://codereview.chromium.org/2844133003/diff/1/mojo/public/js/associated_b... File mojo/public/js/associated_bindings.js (right): https://codereview.chromium.org/2844133003/diff/1/mojo/public/js/associated_b... 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_b... 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/LayoutTe... File third_party/WebKit/LayoutTests/mojo/associated_binding.html (right): https://codereview.chromium.org/2844133003/diff/1/third_party/WebKit/LayoutTe... 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/LayoutTe... 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.
The CQ bit was checked by wangjimmy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2844133003/diff/1/mojo/public/js/associated_b... File mojo/public/js/associated_bindings.js (right): https://codereview.chromium.org/2844133003/diff/1/mojo/public/js/associated_b... 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_b... 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/LayoutTe... File third_party/WebKit/LayoutTests/mojo/associated_binding.html (right): https://codereview.chromium.org/2844133003/diff/1/third_party/WebKit/LayoutTe... 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/LayoutTe... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
LGTM with one comment. https://codereview.chromium.org/2844133003/diff/20001/mojo/public/js/associat... File mojo/public/js/associated_bindings.js (right): https://codereview.chromium.org/2844133003/diff/20001/mojo/public/js/associat... mojo/public/js/associated_bindings.js:242: this.bindingType = AssociatedBinding; Please make this a private member.
The CQ bit was checked by wangjimmy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2844133003/diff/20001/mojo/public/js/associat... File mojo/public/js/associated_bindings.js (right): https://codereview.chromium.org/2844133003/diff/20001/mojo/public/js/associat... 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.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by wangjimmy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by wangjimmy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wangjimmy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2844133003/#ps80001 (title: "Make bindingType private and passed it into BindingSetEntry")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493608288919720,
"parent_rev": "612ac411762985a97166c4144ff0d049716a80db", "commit_rev":
"46cdc9add2a99d8e664df0e8ed4e55df530c48d1"}
Message was sent while issue was closed.
Description was changed from ========== Add associated binding set. Add associated_binding.html layout test. BUG=695635 ========== to ========== Add associated binding set. Add associated_binding.html layout test. BUG=695635 Review-Url: https://codereview.chromium.org/2844133003 Cr-Commit-Position: refs/heads/master@{#468288} Committed: https://chromium.googlesource.com/chromium/src/+/46cdc9add2a99d8e664df0e8ed4e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/46cdc9add2a99d8e664df0e8ed4e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
