Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2163393002:
DevTools: Give autocomplete suggestions even after brackets (Closed)
|
Created:
4 years, 5 months ago by einbinder Modified:
4 years, 4 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, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Give autocomplete suggestions even after brackets
BUG=
Committed: https://crrev.com/77811a237b6865e5b41af0d031f809d11d00b0f9
Cr-Commit-Position: refs/heads/master@{#408776}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Style and apostrophe test #
Total comments: 10
Patch Set 3 : style, race condition #
Total comments: 1
Patch Set 4 : Understand promises #
Total comments: 8
Patch Set 5 : Nicer sequential #
Messages
Total messages: 19 (4 generated)
einbinder@chromium.org changed reviewers: + luoe@chromium.org, lushnikov@chromium.org
https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html (right): https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:40: testCompletions("window.", "do", ["document"]), can we evaluate some object in the page and run completion test over it? This way we won't need to revisit the test once the webplatform evolves and adds some new properties to the window object https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js (right): https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:207: var expressionRange = wordRange.startContainer.rangeOfWord(wordRange.startOffset, " =:({;,!+-*/&|^<>`", proxyElement, "backward"); did you test this new apostrophe? https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:211: for (var i = expressionString.length - 1; i >= 0; i--) { 1. let's extract "i" outside of the loop and name it with a full word ("index"?) 2. I'd convert this into a while loop: var bracketCount = 0; var index = expressionString.length; while (bracketCount >= 0 && index >= 0) { --index; ... } https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:212: switch (expressionString.charAt(i)) { converting this into if statement would be easier to read: if (char === "[" and index < expressionString.length < 1) --bracketCount; else if (char === "]") ++bracketCount; https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:217: // Allow an open bracket at the end for property completion nit: comments should finish with "."
Seems good to me after lushnikov's comments.
https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html (right): https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:40: testCompletions("window.", "do", ["document"]), On 2016/07/20 23:37:54, lushnikov wrote: > can we evaluate some object in the page and run completion test over it? > > This way we won't need to revisit the test once the > webplatform evolves and adds some new properties to the window > object It is a white list so it won't be affected unless they remove window.document https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js (right): https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:207: var expressionRange = wordRange.startContainer.rangeOfWord(wordRange.startOffset, " =:({;,!+-*/&|^<>`", proxyElement, "backward"); On 2016/07/20 23:37:54, lushnikov wrote: > did you test this new apostrophe? Done. https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:211: for (var i = expressionString.length - 1; i >= 0; i--) { On 2016/07/20 23:37:54, lushnikov wrote: > 1. let's extract "i" outside of the loop and name it with a full word ("index"?) > 2. I'd convert this into a while loop: > > var bracketCount = 0; > var index = expressionString.length; > while (bracketCount >= 0 && index >= 0) { > --index; > ... > } Done. https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:212: switch (expressionString.charAt(i)) { On 2016/07/20 23:37:54, lushnikov wrote: > converting this into if statement would be easier to read: > > if (char === "[" and index < expressionString.length < 1) > --bracketCount; > else if (char === "]") > ++bracketCount; Done. https://codereview.chromium.org/2163393002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:217: // Allow an open bracket at the end for property completion On 2016/07/20 23:37:54, lushnikov wrote: > nit: comments should finish with "." Done.
https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html (right): https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:19: return new Promise(resolve => { style: we don't use multiline arrow functions for now, let's extract into a named function https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:30: var found; no need to declare "found" beforehand https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:32: found = false; var found = completions.indexOf(expected[i]) !== -1; https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:48: testCompletions("window.", "do", ["document"]), it's daunting to write a huge Promise.all, but this looks flaky at first glance (who guarantees the order of execution?) https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js (right): https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:224: expressionString = expressionString.substr(index + 1); nit: we tend to use substring in all cases where possible
https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html (right): https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:19: return new Promise(resolve => { On 2016/07/25 18:12:29, lushnikov wrote: > style: we don't use multiline arrow functions for now, let's extract into a > named function Done. https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:30: var found; On 2016/07/25 18:12:29, lushnikov wrote: > no need to declare "found" beforehand Done. https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:32: found = false; On 2016/07/25 18:12:29, lushnikov wrote: > var found = completions.indexOf(expected[i]) !== -1; Done. https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:48: testCompletions("window.", "do", ["document"]), On 2016/07/25 18:12:29, lushnikov wrote: > it's daunting to write a huge Promise.all, but this looks flaky at first glance > (who guarantees the order of execution?) I misread Promise.all docs, done. https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js (right): https://codereview.chromium.org/2163393002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/ExecutionContextSelector.js:224: expressionString = expressionString.substr(index + 1); On 2016/07/25 18:12:29, lushnikov wrote: > nit: we tend to use substring in all cases where possible Done.
https://codereview.chromium.org/2163393002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html (right): https://codereview.chromium.org/2163393002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:48: testCompletions("window.", "do", ["document"]), still, not really sequential ;)
This ready for another review?
lgtm https://codereview.chromium.org/2163393002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html (right): https://codereview.chromium.org/2163393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:19: return innerCompletions; Let's nest it less var div = document.createElement("div"); .... var callback; var promise = new Promise(fulfill => callback = fulfill); WebInspector.ExecutionContextSelector.completionsForTextPromptInCurrentContext(...., checkExpected); return promise; function checkExpected(completions) { .... callback(); } https://codereview.chromium.org/2163393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:42: return tests.length && new Promise(tests.shift()).then(()=>sequential(tests)); fun sequential(tests) { var promise = Promise.resolve(); for (var test of tests) promise = promise.then(test); return promise; } https://codereview.chromium.org/2163393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:46: testCompletions("window.", "do", ["document"]), () => testCompletions("window.", "do", ["document"]), ... () => ...
LGTM https://codereview.chromium.org/2163393002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html (right): https://codereview.chromium.org/2163393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:54: testCompletions("templateString`asdf`","should",["shouldNotFindThis"]) Nit: There should be spaces after the commas.
https://codereview.chromium.org/2163393002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html (right): https://codereview.chromium.org/2163393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:19: return innerCompletions; On 2016/07/29 at 01:56:54, lushnikov wrote: > Let's nest it less > > var div = document.createElement("div"); > .... > > var callback; > var promise = new Promise(fulfill => callback = fulfill); > WebInspector.ExecutionContextSelector.completionsForTextPromptInCurrentContext(...., checkExpected); > return promise; > > function checkExpected(completions) > { > .... > callback(); > } Done. https://codereview.chromium.org/2163393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:42: return tests.length && new Promise(tests.shift()).then(()=>sequential(tests)); On 2016/07/29 at 01:56:55, lushnikov wrote: > fun sequential(tests) > { > var promise = Promise.resolve(); > for (var test of tests) > promise = promise.then(test); > return promise; > } Done. https://codereview.chromium.org/2163393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:46: testCompletions("window.", "do", ["document"]), On 2016/07/29 at 01:56:55, lushnikov wrote: > () => testCompletions("window.", "do", ["document"]), > ... > () => ... Done. https://codereview.chromium.org/2163393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/console/console-correct-suggestions.html:54: testCompletions("templateString`asdf`","should",["shouldNotFindThis"]) On 2016/07/29 at 02:03:38, luoe wrote: > Nit: There should be spaces after the commas. Done.
The CQ bit was checked by einbinder@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org, luoe@chromium.org Link to the patchset: https://codereview.chromium.org/2163393002/#ps80001 (title: "Nicer sequential")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Give autocomplete suggestions even after brackets BUG= ========== to ========== DevTools: Give autocomplete suggestions even after brackets BUG= Committed: https://crrev.com/77811a237b6865e5b41af0d031f809d11d00b0f9 Cr-Commit-Position: refs/heads/master@{#408776} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/77811a237b6865e5b41af0d031f809d11d00b0f9 Cr-Commit-Position: refs/heads/master@{#408776} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
