Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2639703002:
DevTools: Console: Provide autocompletions for Maps (Closed)
|
Created:
3 years, 11 months ago by einbinder Modified:
3 years, 11 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Console: Provide autocompletions for Maps
BUG=659152
Review-Url: https://codereview.chromium.org/2639703002
Cr-Original-Commit-Position: refs/heads/master@{#445811}
Committed: https://chromium.googlesource.com/chromium/src/+/a2ab5d688b1b01c2fa805a57762aa9d1e2c4499c
Review-Url: https://codereview.chromium.org/2639703002
Cr-Commit-Position: refs/heads/master@{#445928}
Committed: https://chromium.googlesource.com/chromium/src/+/7011986566d75140d56f66ae98129acbb9196a0d
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use [[Entries]] instead of preview #
Total comments: 6
Patch Set 3 : speling #Patch Set 4 : doc #
Total comments: 8
Patch Set 5 : Add delete #
Total comments: 2
Patch Set 6 : Don't use natural order comparator when there are too many items #Patch Set 7 : New RemoteObject api #
Messages
Total messages: 37 (20 generated)
einbinder@chromium.org changed reviewers: + dgozman@chromium.org
ptal
This needs a screenshot. https://codereview.chromium.org/2639703002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js (right): https://codereview.chromium.org/2639703002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:19: return Components.JavaScriptAutocomplete.completionsForExpression(clippedExpression, query, force) Should this be Promise.all instead? https://codereview.chromium.org/2639703002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:72: * @param {?SDK.RemoteObject} result indentation is off https://codereview.chromium.org/2639703002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:76: if (!result || !!exceptionDetails || !result.preview || result.preview.subtype !== 'map') { Don't use preview: - subtype is available in RemoteObject; - it's not reliable (changes often); - it's limited to ~100 items, so you may just miss completions. Instead, get the internal properties which contain all entries.
http://i.imgur.com/0EiBOuC.png http://i.imgur.com/HD1so0P.png https://codereview.chromium.org/2639703002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js (right): https://codereview.chromium.org/2639703002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:19: return Components.JavaScriptAutocomplete.completionsForExpression(clippedExpression, query, force) On 2017/01/18 at 17:02:39, dgozman wrote: > Should this be Promise.all instead? The promise has already been triggered. Promise.all seems equally clumsy. https://codereview.chromium.org/2639703002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:72: * @param {?SDK.RemoteObject} result On 2017/01/18 at 17:02:39, dgozman wrote: > indentation is off Fixed. https://codereview.chromium.org/2639703002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:76: if (!result || !!exceptionDetails || !result.preview || result.preview.subtype !== 'map') { On 2017/01/18 at 17:02:39, dgozman wrote: > Don't use preview: > - subtype is available in RemoteObject; > - it's not reliable (changes often); > - it's limited to ~100 items, so you may just miss completions. > Instead, get the internal properties which contain all entries. Done.
The CQ bit was checked by einbinder@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_...)
luoe@chromium.org changed reviewers: + luoe@chromium.org
https://codereview.chromium.org/2639703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js (right): https://codereview.chromium.org/2639703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:85: result.getOwnPropertiesPromise().then(extractEntriesProperty); Can we make this a method in RemoteObject.js as getEntriesPromise()? https://codereview.chromium.org/2639703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:103: * @this {!Array<{key:?, value:?}>} @return ? https://codereview.chromium.org/2639703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:122: var qouteChar = '"'; nit: quoteChar
https://codereview.chromium.org/2639703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js (right): https://codereview.chromium.org/2639703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:85: result.getOwnPropertiesPromise().then(extractEntriesProperty); On 2017/01/19 at 18:54:21, luoe wrote: > Can we make this a method in RemoteObject.js as getEntriesPromise()? This only gets the entries from maps with keys as strings. It seems too specific to put in RemoteObject. https://codereview.chromium.org/2639703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:103: * @this {!Array<{key:?, value:?}>} On 2017/01/19 at 18:54:20, luoe wrote: > @return ? Done. https://codereview.chromium.org/2639703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:122: var qouteChar = '"'; On 2017/01/19 at 18:54:20, luoe wrote: > nit: quoteChar Done.
https://codereview.chromium.org/2639703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js (right): https://codereview.chromium.org/2639703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:28: Components.JavaScriptAutocomplete._clipExpression = function(text, allowEndingBracket) { You always pass |true| as a second parameter. https://codereview.chromium.org/2639703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:65: var mapMatch = text.match(/\.\s*[gs]et\s*\(\s*$/); Support delete as well. https://codereview.chromium.org/2639703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:71: var fufill; typo: fufill https://codereview.chromium.org/2639703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:151: suggestions[0].subtitle = 'Keys'; UIString('Keys')
https://codereview.chromium.org/2639703002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js (right): https://codereview.chromium.org/2639703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:28: Components.JavaScriptAutocomplete._clipExpression = function(text, allowEndingBracket) { On 2017/01/20 at 00:30:32, dgozman wrote: > You always pass |true| as a second parameter. Fixed. https://codereview.chromium.org/2639703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:65: var mapMatch = text.match(/\.\s*[gs]et\s*\(\s*$/); On 2017/01/20 at 00:30:32, dgozman wrote: > Support delete as well. Done. https://codereview.chromium.org/2639703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:71: var fufill; On 2017/01/20 at 00:30:32, dgozman wrote: > typo: fufill Done. https://codereview.chromium.org/2639703002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:151: suggestions[0].subtitle = 'Keys'; On 2017/01/20 at 00:30:32, dgozman wrote: > UIString('Keys') Done.
The CQ bit was checked by einbinder@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...
lgtm https://codereview.chromium.org/2639703002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js (right): https://codereview.chromium.org/2639703002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:130: var keys = rawKeys.sort(String.naturalOrderComparator).map(key => quoteChar + key + quoteChar + endChar); Let's not use naturalOrderComparator for large arrays.
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 einbinder@chromium.org
https://codereview.chromium.org/2639703002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js (right): https://codereview.chromium.org/2639703002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/JavaScriptAutocomplete.js:130: var keys = rawKeys.sort(String.naturalOrderComparator).map(key => quoteChar + key + quoteChar + endChar); On 2017/01/23 at 23:57:49, dgozman wrote: > Let's not use naturalOrderComparator for large arrays. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2639703002/#ps100001 (title: "Don't use natural order comparator when there are too many items")
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": 100001, "attempt_start_ts": 1485284791472720,
"parent_rev": "a81c185f34b34ef8410239506825b185b332c00b", "commit_rev":
"a2ab5d688b1b01c2fa805a57762aa9d1e2c4499c"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Console: Provide autocompletions for Maps BUG=659152 ========== to ========== DevTools: Console: Provide autocompletions for Maps BUG=659152 Review-Url: https://codereview.chromium.org/2639703002 Cr-Commit-Position: refs/heads/master@{#445811} Committed: https://chromium.googlesource.com/chromium/src/+/a2ab5d688b1b01c2fa805a57762a... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/a2ab5d688b1b01c2fa805a57762a...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2656683003/ by chenwilliam@chromium.org. The reason for reverting is: Causing closure compilation failure on main waterfall.
fixed closure failure
The CQ bit was checked by einbinder@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2639703002/#ps120001 (title: "New RemoteObject api")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by einbinder@chromium.org
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": 120001, "attempt_start_ts": 1485303369578880,
"parent_rev": "e0e4486e66cb8c74dc8616373b1cf598dfca3fe6", "commit_rev":
"7011986566d75140d56f66ae98129acbb9196a0d"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Console: Provide autocompletions for Maps BUG=659152 Review-Url: https://codereview.chromium.org/2639703002 Cr-Commit-Position: refs/heads/master@{#445811} Committed: https://chromium.googlesource.com/chromium/src/+/a2ab5d688b1b01c2fa805a57762a... ========== to ========== DevTools: Console: Provide autocompletions for Maps BUG=659152 Review-Url: https://codereview.chromium.org/2639703002 Cr-Original-Commit-Position: refs/heads/master@{#445811} Committed: https://chromium.googlesource.com/chromium/src/+/a2ab5d688b1b01c2fa805a57762a... Review-Url: https://codereview.chromium.org/2639703002 Cr-Commit-Position: refs/heads/master@{#445928} Committed: https://chromium.googlesource.com/chromium/src/+/7011986566d75140d56f66ae9812... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/7011986566d75140d56f66ae9812... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
