Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902583002:
Add some closureised JS bindings for DevTools for use by headless embedder (Closed)
|
Created:
3 years, 7 months ago by alex clarke (OOO till 29th) Modified:
3 years, 6 months ago CC:
chromium-reviews, Dan Beam, dbeam+watch-closure_chromium.org, devtools-reviews_chromium.org, jlklein+watch-closure_chromium.org, pfeldman, vitalyp+closure_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd some closureised JS bindings for DevTools.
Sadly these don't support minification because the json objects get
mangled. We could fix that however if it becomes necessary.
BUG=546953
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2902583002
Cr-Commit-Position: refs/heads/master@{#478249}
Committed: https://chromium.googlesource.com/chromium/src/+/1a69b3b221aed4b1ad88d7446aa22cf3d879bfcf
Patch Set 1 #Patch Set 2 : Remove unwanted change #Patch Set 3 : Add missing .grd file #Patch Set 4 : Add missing js file #Patch Set 5 : Don't run the test on windows because js_binary doesn't work #
Total comments: 48
Patch Set 6 : Responding to feedback #Patch Set 7 : Try to fix win.clang #Patch Set 8 : Rebased #Patch Set 9 : Fix the test after rebasing #
Total comments: 4
Patch Set 10 : Refactored the test #
Total comments: 4
Patch Set 11 : Update indent #Patch Set 12 : Rebased #
Total comments: 2
Patch Set 13 : Fix object type #Patch Set 14 : Fix some type annotations #Patch Set 15 : Use goog.forwardDeclare where needed #
Total comments: 21
Patch Set 16 : Make the JS better #
Total comments: 14
Patch Set 17 : More ES6 goodness #Patch Set 18 : Fix error in Connection.onMessage_ #Patch Set 19 : Same fix again to another instance #
Total comments: 2
Patch Set 20 : Fix a few JS lint warnings with the generated code. #Patch Set 21 : Try and fix python test #Messages
Total messages: 106 (83 generated)
Description was changed from ========== Add some closureised JS bindings for DevTools. Sadly these don't support minification because the json objects get mangled. We could fix that however if it becomes necessary. BUG= ========== to ========== Add some closureised JS bindings for DevTools. Sadly these don't support minification because the json objects get mangled. We could fix that however if it becomes necessary. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Add some closureised JS bindings for DevTools. Sadly these don't support minification because the json objects get mangled. We could fix that however if it becomes necessary. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add some closureised JS bindings for DevTools. Sadly these don't support minification because the json objects get mangled. We could fix that however if it becomes necessary. BUG=546953 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
alexclarke@chromium.org changed reviewers: + skyostil@chromium.org
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 alexclarke@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: 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_...)
The CQ bit was checked by alexclarke@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: 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_...)
The CQ bit was checked by alexclarke@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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by alexclarke@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...
skyostil@chromium.org changed reviewers: + aerotwist@chromium.org
Thanks, looks great! Added some initial thoughts. https://codereview.chromium.org/2902583002/diff/80001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2902583002/diff/80001/headless/BUILD.gn#newco... headless/BUILD.gn:166: [ "$target_gen_dir/public/devtools/domains/" + domain + ".js" ] Could we put these in //headless/public/devtools_js or something to make more obvious this is somewhat standalone and to make it clearer what the interface from here to the transport it? (A README.md wouldn't hurt either.) https://codereview.chromium.org/2902583002/diff/80001/headless/BUILD.gn#newco... headless/BUILD.gn:536: "create_renaming_reports", Do we need the report if we don't do renaming? https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... File headless/lib/browser/devtools_api/client_api_generator.py (right): https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/client_api_generator.py:123: 'js_type': '!goog.DevTools.%s.%s' % (domain['domain'], type['id']), Is the goog namespace the one we should be using? What's the convention here? https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/client_api_generator.py:325: def SynthesizeJsConstructorArgs(properties): Please add some coverage in update client_api_generator_test.py too. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/client_api_generator.py:431: js_dependencies = deps |js_dependencies| unused? https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... File headless/lib/browser/devtools_api/devtools_connection.js (right): https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:6: * @fileoverview The DevTools Comment could use expanding :) https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:14: * @class Bindings for the {{domain.domain}} DevTools Domain. This comment seems wrong. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:48: this.transport_.onmessage = this.OnMessage_.bind(this); nit: onMessage https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:60: goog.DevTools.Connection.prototype.AddEventListener = function(eventName, Should we support removing listeners too? nit: addEventListener https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:77: goog.DevTools.Connection.prototype.SendDevToolsMessage = function(method, nit: sendDevToolsMessage https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:83: this.commandId_ += 2; This could use a comment saying why +2 https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:101: console.log('Unrecognized id:' + json_message); console.error? Or just throw an error? https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:109: console.log('Bad message:' + json_message); Ditto. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:114: var listeners = this.eventListeners_[method]; let for consistency? https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... File headless/lib/browser/devtools_api/domain_js.template (right): https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/domain_js.template:6: * @fileoverview Generated DevTools bindings for the {{domain.domain}} Domain. How do we handle experimental things currently? Maybe we should skip them until we decide how to expose them? https://codereview.chromium.org/2902583002/diff/80001/headless/test/bindings_... File headless/test/bindings_test.js (right): https://codereview.chromium.org/2902583002/diff/80001/headless/test/bindings_... headless/test/bindings_test.js:22: DOM.Enable(); We should wait for this to finish I think. https://codereview.chromium.org/2902583002/diff/80001/headless/test/headless_... File headless/test/headless_js_bindings_browsertest.cc (right): https://codereview.chromium.org/2902583002/diff/80001/headless/test/headless_... headless/test/headless_js_bindings_browsertest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. ++year https://codereview.chromium.org/2902583002/diff/80001/headless/test/test_harn... File headless/test/test_harness.js (right): https://codereview.chromium.org/2902583002/diff/80001/headless/test/test_harn... headless/test/test_harness.js:1: // Copyright 2017 The Chromium Authors. All rights reserved. Could we reuse an existing test framework (jstest?) instead of rolling our own? I think that's what's recommended for layout tests these days. https://codereview.chromium.org/2902583002/diff/80001/third_party/closure_com... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2902583002/diff/80001/third_party/closure_com... third_party/closure_compiler/compile_js.gni:17: # deps: I think renaming this to js_deps instead would make deps work like you'd expect from other gn rules.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... File headless/lib/browser/devtools_api/devtools_connection.js (right): https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:60: goog.DevTools.Connection.prototype.AddEventListener = function(eventName, On 2017/05/24 09:16:21, Sami wrote: > Should we support removing listeners too? > > nit: addEventListener I'd love to see removeEventListener added here. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:77: goog.DevTools.Connection.prototype.SendDevToolsMessage = function(method, On 2017/05/24 09:16:21, Sami wrote: > nit: sendDevToolsMessage It'd be worth checking whether the DevTools API (or DevTools itself) marshals requests into a chain, or whether it lets you run multiple requests in parallel like this. If not, could we potentially see race conditions? https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:114: var listeners = this.eventListeners_[method]; On 2017/05/24 09:16:21, Sami wrote: > let for consistency? May as well go full const here; it looks like the ref is immutable.
alexclarke@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam@ for the third_party/closure_compiler/compile_js.gni change https://codereview.chromium.org/2902583002/diff/80001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2902583002/diff/80001/headless/BUILD.gn#newco... headless/BUILD.gn:166: [ "$target_gen_dir/public/devtools/domains/" + domain + ".js" ] On 2017/05/24 09:16:21, Sami wrote: > Could we put these in //headless/public/devtools_js or something to make more > obvious this is somewhat standalone and to make it clearer what the interface > from here to the transport it? (A README.md wouldn't hurt either.) They need to be in the gen directory right? I can get them to go into .../gen/headless/public/devtools_js https://codereview.chromium.org/2902583002/diff/80001/headless/BUILD.gn#newco... headless/BUILD.gn:536: "create_renaming_reports", On 2017/05/24 09:16:21, Sami wrote: > Do we need the report if we don't do renaming? Done. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... File headless/lib/browser/devtools_api/client_api_generator.py (right): https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/client_api_generator.py:123: 'js_type': '!goog.DevTools.%s.%s' % (domain['domain'], type['id']), On 2017/05/24 09:16:21, Sami wrote: > Is the goog namespace the one we should be using? What's the convention here? Well we are google so goog seems appropriate? https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/client_api_generator.py:325: def SynthesizeJsConstructorArgs(properties): On 2017/05/24 09:16:21, Sami wrote: > Please add some coverage in update client_api_generator_test.py too. We don't need this anymore, removing. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/client_api_generator.py:431: js_dependencies = deps On 2017/05/24 09:16:21, Sami wrote: > |js_dependencies| unused? Done. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... File headless/lib/browser/devtools_api/devtools_connection.js (right): https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:6: * @fileoverview The DevTools On 2017/05/24 09:16:21, Sami wrote: > Comment could use expanding :) Done. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:14: * @class Bindings for the {{domain.domain}} DevTools Domain. On 2017/05/24 09:16:21, Sami wrote: > This comment seems wrong. Done. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:48: this.transport_.onmessage = this.OnMessage_.bind(this); On 2017/05/24 09:16:21, Sami wrote: > nit: onMessage We defined this is as onmessage in the TabSocket. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:60: goog.DevTools.Connection.prototype.AddEventListener = function(eventName, On 2017/05/24 09:16:21, Sami wrote: > Should we support removing listeners too? > > nit: addEventListener Done. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:77: goog.DevTools.Connection.prototype.SendDevToolsMessage = function(method, On 2017/05/24 09:16:21, Sami wrote: > nit: sendDevToolsMessage Done. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:77: goog.DevTools.Connection.prototype.SendDevToolsMessage = function(method, On 2017/05/24 11:06:42, aerotwist wrote: > On 2017/05/24 09:16:21, Sami wrote: > > nit: sendDevToolsMessage > > It'd be worth checking whether the DevTools API (or DevTools itself) marshals > requests into a chain, or whether it lets you run multiple requests in parallel > like this. If not, could we potentially see race conditions? In theory commands sent in the order A,B,C are processed in that order but some are processed by the browser process and some by the renderer. So race conditions are a thing and basically you'd need to use promise chaining to be sure stuff happens in the expected order. This is the case for existing bindings such as https://www.npmjs.com/package/chrome-remote-interface. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:83: this.commandId_ += 2; On 2017/05/24 09:16:21, Sami wrote: > This could use a comment saying why +2 Done. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:101: console.log('Unrecognized id:' + json_message); On 2017/05/24 09:16:21, Sami wrote: > console.error? Or just throw an error? Done. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:109: console.log('Bad message:' + json_message); On 2017/05/24 09:16:21, Sami wrote: > Ditto. Done. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/devtools_connection.js:114: var listeners = this.eventListeners_[method]; On 2017/05/24 09:16:21, Sami wrote: > let for consistency? Done. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... File headless/lib/browser/devtools_api/domain_js.template (right): https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/domain_js.template:6: * @fileoverview Generated DevTools bindings for the {{domain.domain}} Domain. On 2017/05/24 09:16:21, Sami wrote: > How do we handle experimental things currently? Maybe we should skip them until > we decide how to expose them? I guess we should just drop them and you have to use the lower level Connection class if you want to touch them. https://codereview.chromium.org/2902583002/diff/80001/headless/test/bindings_... File headless/test/bindings_test.js (right): https://codereview.chromium.org/2902583002/diff/80001/headless/test/bindings_... headless/test/bindings_test.js:22: DOM.Enable(); On 2017/05/24 09:16:21, Sami wrote: > We should wait for this to finish I think. Done. https://codereview.chromium.org/2902583002/diff/80001/headless/test/headless_... File headless/test/headless_js_bindings_browsertest.cc (right): https://codereview.chromium.org/2902583002/diff/80001/headless/test/headless_... headless/test/headless_js_bindings_browsertest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/05/24 09:16:22, Sami wrote: > ++year Done. https://codereview.chromium.org/2902583002/diff/80001/headless/test/test_harn... File headless/test/test_harness.js (right): https://codereview.chromium.org/2902583002/diff/80001/headless/test/test_harn... headless/test/test_harness.js:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/24 09:16:22, Sami wrote: > Could we reuse an existing test framework (jstest?) instead of rolling our own? Is that in chromium somewhere? > I think that's what's recommended for layout tests these days. Most existing js test frameworks I'm familiar with (e.g. Karma) are inappropriate here since they typically have their own webkit backend. https://codereview.chromium.org/2902583002/diff/80001/third_party/closure_com... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2902583002/diff/80001/third_party/closure_com... third_party/closure_compiler/compile_js.gni:17: # deps: On 2017/05/24 09:16:22, Sami wrote: > I think renaming this to js_deps instead would make deps work like you'd expect > from other gn rules. +dbeam@ WDYT?
The CQ bit was checked by alexclarke@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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by alexclarke@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: 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_...)
The CQ bit was checked by alexclarke@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: 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 alexclarke@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: 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...)
https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... File headless/lib/browser/devtools_api/client_api_generator.py (right): https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/client_api_generator.py:123: 'js_type': '!goog.DevTools.%s.%s' % (domain['domain'], type['id']), On 2017/05/24 11:38:14, alex clarke wrote: > On 2017/05/24 09:16:21, Sami wrote: > > Is the goog namespace the one we should be using? What's the convention here? > > Well we are google so goog seems appropriate? Let's use the chrome namespace since this is more a Chrome API than a Google API and there's some precedent too (chrome.gpuBenchmarking). https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... File headless/lib/browser/devtools_api/domain_js.template (right): https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/domain_js.template:6: * @fileoverview Generated DevTools bindings for the {{domain.domain}} Domain. On 2017/05/24 11:38:15, alex clarke wrote: > On 2017/05/24 09:16:21, Sami wrote: > > How do we handle experimental things currently? Maybe we should skip them > until > > we decide how to expose them? > > I guess we should just drop them and you have to use the lower level Connection > class if you want to touch them. Right, let's not generate anything for them now and have a TODO somewhere? https://codereview.chromium.org/2902583002/diff/80001/headless/test/test_harn... File headless/test/test_harness.js (right): https://codereview.chromium.org/2902583002/diff/80001/headless/test/test_harn... headless/test/test_harness.js:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/24 11:38:17, alex clarke wrote: > On 2017/05/24 09:16:22, Sami wrote: > > Could we reuse an existing test framework (jstest?) instead of rolling our > own? > Is that in chromium somewhere? > > > I think that's what's recommended for layout tests these days. > > Most existing js test frameworks I'm familiar with (e.g. Karma) are > inappropriate here since they typically have their own webkit backend. Sorry, meant testharness.js. See here: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... https://codereview.chromium.org/2902583002/diff/160001/headless/test/headless... File headless/test/headless_js_bindings_browsertest.cc (right): https://codereview.chromium.org/2902583002/diff/160001/headless/test/headless... headless/test/headless_js_bindings_browsertest.cc:52: "var goog = {};" + ResourceBundle::GetSharedInstance() Could we make conditionally initializing the namespace part of the bindings themselves? https://codereview.chromium.org/2902583002/diff/160001/headless/test/headless... headless/test/headless_js_bindings_browsertest.cc:91: fprintf(stdout, "%s\n", message.c_str()); Probably want to remove this :)
PTAL https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... File headless/lib/browser/devtools_api/client_api_generator.py (right): https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/client_api_generator.py:123: 'js_type': '!goog.DevTools.%s.%s' % (domain['domain'], type['id']), On 2017/05/25 17:53:35, Sami wrote: > On 2017/05/24 11:38:14, alex clarke wrote: > > On 2017/05/24 09:16:21, Sami wrote: > > > Is the goog namespace the one we should be using? What's the convention > here? > > > > Well we are google so goog seems appropriate? > > Let's use the chrome namespace since this is more a Chrome API than a Google API > and there's some precedent too (chrome.gpuBenchmarking). As discussed offline we had to use chromium instead because there was some built in extern for chrome. https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... File headless/lib/browser/devtools_api/domain_js.template (right): https://codereview.chromium.org/2902583002/diff/80001/headless/lib/browser/de... headless/lib/browser/devtools_api/domain_js.template:6: * @fileoverview Generated DevTools bindings for the {{domain.domain}} Domain. On 2017/05/25 17:53:35, Sami wrote: > On 2017/05/24 11:38:15, alex clarke wrote: > > On 2017/05/24 09:16:21, Sami wrote: > > > How do we handle experimental things currently? Maybe we should skip them > > until > > > we decide how to expose them? > > > > I guess we should just drop them and you have to use the lower level > Connection > > class if you want to touch them. > > Right, let's not generate anything for them now and have a TODO somewhere? Done. https://codereview.chromium.org/2902583002/diff/80001/headless/test/test_harn... File headless/test/test_harness.js (right): https://codereview.chromium.org/2902583002/diff/80001/headless/test/test_harn... headless/test/test_harness.js:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/05/25 17:53:35, Sami wrote: > On 2017/05/24 11:38:17, alex clarke wrote: > > On 2017/05/24 09:16:22, Sami wrote: > > > Could we reuse an existing test framework (jstest?) instead of rolling our > > own? > > Is that in chromium somewhere? > > > > > I think that's what's recommended for layout tests these days. > > > > Most existing js test frameworks I'm familiar with (e.g. Karma) are > > inappropriate here since they typically have their own webkit backend. > > Sorry, meant testharness.js. See here: > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... As discussed offline I refactored the test se we don't need this. https://codereview.chromium.org/2902583002/diff/160001/headless/test/headless... File headless/test/headless_js_bindings_browsertest.cc (right): https://codereview.chromium.org/2902583002/diff/160001/headless/test/headless... headless/test/headless_js_bindings_browsertest.cc:52: "var goog = {};" + ResourceBundle::GetSharedInstance() On 2017/05/25 17:53:35, Sami wrote: > Could we make conditionally initializing the namespace part of the bindings > themselves? Seems we don't need this anymore, possibly because we're using the chomium namespace. https://codereview.chromium.org/2902583002/diff/160001/headless/test/headless... headless/test/headless_js_bindings_browsertest.cc:91: fprintf(stdout, "%s\n", message.c_str()); On 2017/05/25 17:53:35, Sami wrote: > Probably want to remove this :) Acknowledged.
The CQ bit was checked by alexclarke@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: 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_...)
Thanks, lgtm! (Although I still prefer js_deps :) https://codereview.chromium.org/2902583002/diff/180001/headless/lib/browser/d... File headless/lib/browser/devtools_api/devtools_connection.js (right): https://codereview.chromium.org/2902583002/diff/180001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:64: listener) { nit: this line wrapping looks a little off (same below) https://codereview.chromium.org/2902583002/diff/180001/headless/lib/browser/d... File headless/lib/browser/devtools_api/domain_js.template (right): https://codereview.chromium.org/2902583002/diff/180001/headless/lib/browser/d... headless/lib/browser/devtools_api/domain_js.template:41: {% if not "enum" in type %}{% continue %}{% endif %} Please indent the {% %} template tags like we do in the other .template files. It makes it a bit easier to follow the nesting.
https://codereview.chromium.org/2902583002/diff/180001/headless/lib/browser/d... File headless/lib/browser/devtools_api/devtools_connection.js (right): https://codereview.chromium.org/2902583002/diff/180001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:64: listener) { On 2017/05/26 15:55:54, Sami wrote: > nit: this line wrapping looks a little off (same below) Done. https://codereview.chromium.org/2902583002/diff/180001/headless/lib/browser/d... File headless/lib/browser/devtools_api/domain_js.template (right): https://codereview.chromium.org/2902583002/diff/180001/headless/lib/browser/d... headless/lib/browser/devtools_api/domain_js.template:41: {% if not "enum" in type %}{% continue %}{% endif %} On 2017/05/26 15:55:54, Sami wrote: > Please indent the {% %} template tags like we do in the other .template files. > It makes it a bit easier to follow the nesting. Done.
The CQ bit was checked by alexclarke@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: 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...)
The CQ bit was checked by alexclarke@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 alexclarke@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: 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...)
dbeam@ could you take a look at the change in third_party/closure_compiler/compile_js.gni please?
https://codereview.chromium.org/2902583002/diff/220001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2902583002/diff/220001/headless/BUILD.gn#newc... headless/BUILD.gn:213: extra_deps = [ ":gen_devtools_client_api" ] errrr, why can't these just be deps?
https://codereview.chromium.org/2902583002/diff/220001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/2902583002/diff/220001/headless/BUILD.gn#newc... headless/BUILD.gn:213: extra_deps = [ ":gen_devtools_client_api" ] On 2017/06/01 16:51:45, Dan Beam wrote: > errrr, why can't these just be deps? The problem is js_binary (incorrectly IMO) assumes deps are js_library deps but if you have a python script like this it blows up like so: Traceback (most recent call last): File "../../third_party/closure_compiler/js_binary.py", line 86, in <module> sys.exit(main()) File "../../third_party/closure_compiler/js_binary.py", line 64, in main sources = CrawlDepsTree(args.deps, []) + args.sources File "../../third_party/closure_compiler/js_binary.py", line 37, in CrawlDepsTree sources = CrawlDepsTree(new_deps, sources) File "../../third_party/closure_compiler/js_binary.py", line 35, in CrawlDepsTree new_sources, new_deps = ParseDepList(dep) File "../../third_party/closure_compiler/js_binary.py", line 24, in ParseDepList ' is not a js_library target') TypeError: can only concatenate tuple (not "str") to tuple There are a few options for fixing this. Sami prefers s/deps/js_deps in the js_library & js_binary definitions. I added this extra_deps because it's a pattern I've used before in google3. A third option might be to somehow make js-binary smart enough to spot something isn't a js_library but I'm not sure how to do that. What do you think we should do here?
The CQ bit was checked by alexclarke@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.
PING :)
The CQ bit was checked by alexclarke@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...
alexclarke@chromium.org changed reviewers: + dpapad@chromium.org - dbeam@chromium.org
Ah looks like dbeam@ is moving on. dpapad@chromium.org could you please take a look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... File headless/lib/browser/devtools_api/devtools_connection.js (right): https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:16: * @typedef {!function(Object): undefined|!function(string): undefined} "!" not necessary for |function| which is considered by default non-nullable. See https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the..., specifically the part that says "All object types are nullable by default whether or not they are declared with the Nullable operator. An object type is defined as anything except a function, string, number, or boolean." https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:35: /** Nit: Consider compacting those as where possible, as follows: /** @private {number} */ https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:43: * @type {!Object<!chromium.DevTools.CallbackFunction>} Why not using the shorthand syntax? Also why not using a Map instead of a plain Object? @private {!Object<!chromium.DevTools.CallbackFunction>} https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:52: * @type {!Object<!Array<!chromium.DevTools.CallbackFunction>>} Same question about Object vs Map https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:65: * @param {!string} eventName Name of the DevTools protocol event to listen for. "!" not necessary here (and elsewhere). https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:83: chromium.DevTools.Connection.prototype.removeEventListener = function( This is mimicking the addEventListener/removeEventListener API of HTMLElement, which is a known shortcoming of the Web. Explaining below: The misuse of this API is unfortunately far too common. // Add an event listener. foo.addEventListener('some-event', this.someFunction.bind(this)); // Erroneously fail to remove an event listener, because a different function reference is passed. foo.removeEventListener('some-event', this.someFunction.bind(this)); Where it should have been as follows: var boundFunction = this.someFunction.bind(this); foo.addEventListener('some-event', boundFunction); foo.removeEventListener('some-event', boundFunction); So if there is no reason to replicate this API, here is a far better alternative var listenerId = foo.addEventListener('some-event', this.someFunction.bind(this)); foo.removeEventListener(listenerId); Where removeEventListener returns a boolean, with true if the listener was indeed removed. This API is harder to misuse, and notice how the removeEventListener() method does not even need the name of the event anymore. Keeping a Map internally to associate listenerIds (just a number) with listeners is also trivial. You can also avoid a lot of the linear searching that happens in this method via the usage of indexOf. https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:98: * @return {Promise<!Object>} A promise for the results object. !Promise https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:113: JSON.stringify({'method': method, 'id': id, 'params': opt_params})); I suppose that the single quotes here are used because otherwise JS compiler mangles the object names. If that is the case please mention that in the comment, otherwise a future developer might remove them and possibly break things. https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:121: * @param {string} json_message An object containing a DevTools protocol Per JS Styleguide: s/json_message/jsonMessage https://codereview.chromium.org/2902583002/diff/280001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2902583002/diff/280001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:135: "extra_deps", Does JS binary also need to have an extra_deps? Can't users of js_binary create a js_library with the correct extra_deps and add it as a dependency to js_binary's |deps| instead?
Thanks for the JS tips. PTAL :) https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... File headless/lib/browser/devtools_api/devtools_connection.js (right): https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:16: * @typedef {!function(Object): undefined|!function(string): undefined} On 2017/06/06 17:37:46, dpapad wrote: > "!" not necessary for |function| which is considered by default non-nullable. > See > https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the..., > specifically the part that says > > "All object types are nullable by default whether or not they are declared with > the Nullable operator. An object type is defined as anything except a function, > string, number, or boolean." Done. https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:35: /** On 2017/06/06 17:37:45, dpapad wrote: > Nit: Consider compacting those as where possible, as follows: > > /** @private {number} */ Done. https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:43: * @type {!Object<!chromium.DevTools.CallbackFunction>} On 2017/06/06 17:37:46, dpapad wrote: > Why not using the shorthand syntax? Also why not using a Map instead of a plain > Object? > > @private {!Object<!chromium.DevTools.CallbackFunction>} https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:52: * @type {!Object<!Array<!chromium.DevTools.CallbackFunction>>} On 2017/06/06 17:37:46, dpapad wrote: > Same question about Object vs Map Done. https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:65: * @param {!string} eventName Name of the DevTools protocol event to listen for. On 2017/06/06 17:37:46, dpapad wrote: > "!" not necessary here (and elsewhere). Done. https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:83: chromium.DevTools.Connection.prototype.removeEventListener = function( On 2017/06/06 17:37:46, dpapad wrote: > This is mimicking the addEventListener/removeEventListener API of HTMLElement, > which is a known shortcoming of the Web. Explaining below: > > The misuse of this API is unfortunately far too common. > > // Add an event listener. > foo.addEventListener('some-event', this.someFunction.bind(this)); > // Erroneously fail to remove an event listener, because a different function > reference is passed. > foo.removeEventListener('some-event', this.someFunction.bind(this)); > > Where it should have been as follows: > var boundFunction = this.someFunction.bind(this); > foo.addEventListener('some-event', boundFunction); > foo.removeEventListener('some-event', boundFunction); > > > So if there is no reason to replicate this API, here is a far better alternative > > var listenerId = foo.addEventListener('some-event', > this.someFunction.bind(this)); > foo.removeEventListener(listenerId); > > Where removeEventListener returns a boolean, with true if the listener was > indeed removed. This API is harder to misuse, and notice how the > removeEventListener() method does not even need the name of the event anymore. > Keeping a Map internally to associate listenerIds (just a number) with listeners > is also trivial. You can also avoid a lot of the linear searching that happens > in this method via the usage of indexOf. Done. https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:98: * @return {Promise<!Object>} A promise for the results object. On 2017/06/06 17:37:46, dpapad wrote: > !Promise Done. https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:113: JSON.stringify({'method': method, 'id': id, 'params': opt_params})); On 2017/06/06 17:37:46, dpapad wrote: > I suppose that the single quotes here are used because otherwise JS compiler > mangles the object names. If that is the case please mention that in the > comment, otherwise a future developer might remove them and possibly break > things. Done. https://codereview.chromium.org/2902583002/diff/280001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:121: * @param {string} json_message An object containing a DevTools protocol On 2017/06/06 17:37:45, dpapad wrote: > Per JS Styleguide: > s/json_message/jsonMessage Done. https://codereview.chromium.org/2902583002/diff/280001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2902583002/diff/280001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:135: "extra_deps", On 2017/06/06 17:37:46, dpapad wrote: > Does JS binary also need to have an extra_deps? Can't users of js_binary create > a js_library with the correct extra_deps and add it as a dependency to > js_binary's |deps| instead? No we don't strictly need this. I only added it to make the API orthogonal. Lets remove it for now?
The CQ bit was checked by alexclarke@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.
Consider all ES6 related comments optional. https://codereview.chromium.org/2902583002/diff/280001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2902583002/diff/280001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:135: "extra_deps", > No we don't strictly need this. I only added it to make the API orthogonal. Lets remove it for now? Yes, let's defer this until it is actually needed. https://codereview.chromium.org/2902583002/diff/300001/headless/lib/browser/d... File headless/lib/browser/devtools_api/devtools_connection.js (right): https://codereview.chromium.org/2902583002/diff/300001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:88: * @returbn {boolean} Returns true if the event listener was actually removed. s/returnbn/return s/Returns true if/Whether https://codereview.chromium.org/2902583002/diff/300001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:92: let eventName = this.eventListenerIdToEventName_.get(id); Nit (optional): You are using ES6 (aka ES2015) 'let' here, but the rest of this file is using old style prototype to define the Connection class. Have you considered using a proper ES6 class definition instead? FYI, we have an ES6 guide for Chromium JS code at https://chromium.googlesource.com/chromium/src/+/master/docs/es6_chromium.md#..., and I am planning to propose moving more features in the "Allowed list" very soon (probably today). https://codereview.chromium.org/2902583002/diff/300001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:109: method, opt_params) { If you were to use ES6 for declaring optional parameters, you could do the following chromium.DevTools.Connection.prototype.sendDevToolsMessage = function(method, params = {}) { let id = this.commandId_; ... } See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/D.... https://codereview.chromium.org/2902583002/diff/300001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:120: // Note the names are in quites to prevent closure compiler name mangling. s/quites/quotes https://codereview.chromium.org/2902583002/diff/300001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:124: this.pendingCommands_.set(id, resolve); I am assuming that the returned promise can never fail? FWIW, we use a small helper class for similar situations elsewhere in the code, called PromiseResolver (https://cs.chromium.org/chromium/src/ui/webui/resources/js/promise_resolver.js), which allows both resolving/rejecting a promise at a later time. https://codereview.chromium.org/2902583002/diff/300001/headless/test/bindings... File headless/test/bindings_test.js (right): https://codereview.chromium.org/2902583002/diff/300001/headless/test/bindings... headless/test/bindings_test.js:30: DOM.Enable().then(function() { Prefer Promise chaining over nesting where possible. DOM.Enable().then(function() { return DOM.GetDocument({}); }).then(function() { // Create a new div which should trigger the event. let div = document.createElement('div'); document.body.appendChild(div); }); https://codereview.chromium.org/2902583002/diff/300001/headless/test/bindings... headless/test/bindings_test.js:31: DOM.GetDocument({}).then(function(result) { |result| is not being used anywhere. Can you omit it?
PTAL https://codereview.chromium.org/2902583002/diff/300001/headless/lib/browser/d... File headless/lib/browser/devtools_api/devtools_connection.js (right): https://codereview.chromium.org/2902583002/diff/300001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:88: * @returbn {boolean} Returns true if the event listener was actually removed. On 2017/06/07 17:49:23, dpapad wrote: > s/returnbn/return > s/Returns true if/Whether Done. https://codereview.chromium.org/2902583002/diff/300001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:92: let eventName = this.eventListenerIdToEventName_.get(id); On 2017/06/07 17:49:23, dpapad wrote: > Nit (optional): You are using ES6 (aka ES2015) 'let' here, but the rest of this > file is using old style prototype to define the Connection class. Have you > considered using a proper ES6 class definition instead? It sounds like you're OK with ES6 stuff :) ES6style code does look much more readable so lets go with that. > > FYI, we have an ES6 guide for Chromium JS code at > https://chromium.googlesource.com/chromium/src/+/master/docs/es6_chromium.md#..., > and I am planning to propose moving more features in the "Allowed list" very > soon (probably today). https://codereview.chromium.org/2902583002/diff/300001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:109: method, opt_params) { On 2017/06/07 17:49:22, dpapad wrote: > If you were to use ES6 for declaring optional parameters, you could do the > following > > chromium.DevTools.Connection.prototype.sendDevToolsMessage = function(method, > params = {}) { > let id = this.commandId_; > ... > } > > See > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/D.... Done. https://codereview.chromium.org/2902583002/diff/300001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:120: // Note the names are in quites to prevent closure compiler name mangling. On 2017/06/07 17:49:22, dpapad wrote: > s/quites/quotes Done. https://codereview.chromium.org/2902583002/diff/300001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:124: this.pendingCommands_.set(id, resolve); On 2017/06/07 17:49:22, dpapad wrote: > I am assuming that the returned promise can never fail? FWIW, we use a small > helper class for similar situations elsewhere in the code, called > PromiseResolver > (https://cs.chromium.org/chromium/src/ui/webui/resources/js/promise_resolver.js), > which allows both resolving/rejecting a promise at a later time. Interesting. I guess right now that's not super easy to reuse here without adding more js_library definitions. https://codereview.chromium.org/2902583002/diff/300001/headless/test/bindings... File headless/test/bindings_test.js (right): https://codereview.chromium.org/2902583002/diff/300001/headless/test/bindings... headless/test/bindings_test.js:30: DOM.Enable().then(function() { On 2017/06/07 17:49:23, dpapad wrote: > Prefer Promise chaining over nesting where possible. > > DOM.Enable().then(function() { > return DOM.GetDocument({}); > }).then(function() { > // Create a new div which should trigger the event. > let div = document.createElement('div'); > document.body.appendChild(div); > }); Done. https://codereview.chromium.org/2902583002/diff/300001/headless/test/bindings... headless/test/bindings_test.js:31: DOM.GetDocument({}).then(function(result) { On 2017/06/07 17:49:23, dpapad wrote: > |result| is not being used anywhere. Can you omit it? Done.
The CQ bit was checked by alexclarke@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 alexclarke@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 alexclarke@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: 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...)
LGTM https://codereview.chromium.org/2902583002/diff/360001/headless/lib/browser/d... File headless/lib/browser/devtools_api/devtools_connection.js (right): https://codereview.chromium.org/2902583002/diff/360001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:19: class Connection { FYI, yesterday I discovered an issue when using ES6 clasess and --chrome_pass flag, details at https://bugs.chromium.org/p/chromium/issues/detail?id=671426#c20. Use at your own risk (until the linked bug is addressed).
Thanks for the review! https://codereview.chromium.org/2902583002/diff/360001/headless/lib/browser/d... File headless/lib/browser/devtools_api/devtools_connection.js (right): https://codereview.chromium.org/2902583002/diff/360001/headless/lib/browser/d... headless/lib/browser/devtools_api/devtools_connection.js:19: class Connection { On 2017/06/08 17:12:03, dpapad wrote: > FYI, yesterday I discovered an issue when using ES6 clasess and --chrome_pass > flag, details at > https://bugs.chromium.org/p/chromium/issues/detail?id=671426#c20. Use at your > own risk (until the linked bug is addressed). Noted, thanks.
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2902583002/#ps380001 (title: "Fix a few JS lint warnings with the generated code.")
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
Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2902583002/#ps400001 (title: "Try and fix python test")
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": 400001, "attempt_start_ts": 1497000357579120,
"parent_rev": "e2c07e263512a1f828dd3d35b3c6c5d4220c84fe", "commit_rev":
"1a69b3b221aed4b1ad88d7446aa22cf3d879bfcf"}
Message was sent while issue was closed.
Description was changed from ========== Add some closureised JS bindings for DevTools. Sadly these don't support minification because the json objects get mangled. We could fix that however if it becomes necessary. BUG=546953 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add some closureised JS bindings for DevTools. Sadly these don't support minification because the json objects get mangled. We could fix that however if it becomes necessary. BUG=546953 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2902583002 Cr-Commit-Position: refs/heads/master@{#478249} Committed: https://chromium.googlesource.com/chromium/src/+/1a69b3b221aed4b1ad88d7446aa2... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/1a69b3b221aed4b1ad88d7446aa2... |
