Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2439223002:
DevTools: Use grey hint text for applied suggestion in TextPrompt (Closed)
|
Created:
4 years, 2 months ago by einbinder Modified:
4 years ago Reviewers:
lushnikov 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: Use grey hint text for applied suggestion in TextPrompt
Auto completions in TextPrompt used to appear as selected text. Now they
always show up as grey hint text. This is now consistent with the console
prompt and sources panel text editor.
BUG=none
Committed: https://crrev.com/5ba3f4712604e0201423db169c25178db98a16a0
Cr-Commit-Position: refs/heads/master@{#427916}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fix bug with completing in the middle of a prompt #
Total comments: 16
Patch Set 3 : prefixRange #Patch Set 4 : newline for test #
Total comments: 7
Patch Set 5 : Merge #Patch Set 6 : merge #
Total comments: 2
Patch Set 7 : Was removing ghostText twice #
Messages
Total messages: 23 (13 generated)
einbinder@chromium.org changed reviewers: + lushnikov@chromium.org
ptal https://codereview.chromium.org/2439223002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js (right): https://codereview.chromium.org/2439223002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:348: this._autocompleteElement.textContent = this._currentHintText.substring(text.length); broken
Description was changed from ========== DevTools: Use grey hint text for applied suggestion in TextPrompt Auto completions in TextPrompt use to appear as selected text. Now they always show up as grey hint text. This is now consistent with the console prompt and sources panel text editor. BUG=none ========== to ========== DevTools: Use grey hint text for applied suggestion in TextPrompt Auto completions in TextPrompt use to appear as selected text. Now they always show up as grey hint text. This is now consistent with the console prompt and sources panel text editor. BUG=none ==========
https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js (left): https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:349: var hasCommonPrefix = text.startsWith(this._previousText) || this._previousText.startsWith(text); let's get it back! it was so good! https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js (right): https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:350: } nit: "else" on the previous line https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:440: var autocompleteTextLength = (this._autocompleteElement.parentNode) ? this._autocompleteElement.textContent.length : 0; nit: extra () https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:542: var beforeRange = this._createRange(); the calculation of the userEnteredRange should be before bailing out https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:591: * @param {number} start startColumn https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:592: * @param {number} end endColumn https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:594: _select: function(start, end) _setDOMSelection: https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:598: if (node === this._autocompleteElement) !node
I renamed _userEnteredRange -> _prefixRange https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js (left): https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:349: var hasCommonPrefix = text.startsWith(this._previousText) || this._previousText.startsWith(text); On 2016/10/22 at 02:15:10, lushnikov wrote: > let's get it back! it was so good! Done. https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js (right): https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:350: } On 2016/10/22 at 02:15:10, lushnikov wrote: > nit: "else" on the previous line Done. https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:440: var autocompleteTextLength = (this._autocompleteElement.parentNode) ? this._autocompleteElement.textContent.length : 0; On 2016/10/22 at 02:15:10, lushnikov wrote: > nit: extra () Done. https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:542: var beforeRange = this._createRange(); On 2016/10/22 at 02:15:10, lushnikov wrote: > the calculation of the userEnteredRange should be before bailing out Done. https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:591: * @param {number} start On 2016/10/22 at 02:15:10, lushnikov wrote: > startColumn Done. https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:592: * @param {number} end On 2016/10/22 at 02:15:10, lushnikov wrote: > endColumn Done. https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:594: _select: function(start, end) On 2016/10/22 at 02:15:10, lushnikov wrote: > _setDOMSelection: Done. https://codereview.chromium.org/2439223002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:598: if (node === this._autocompleteElement) On 2016/10/22 at 02:15:10, lushnikov wrote: > !node 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== DevTools: Use grey hint text for applied suggestion in TextPrompt Auto completions in TextPrompt use to appear as selected text. Now they always show up as grey hint text. This is now consistent with the console prompt and sources panel text editor. BUG=none ========== to ========== DevTools: Use grey hint text for applied suggestion in TextPrompt Auto completions in TextPrompt used to appear as selected text. Now they always show up as grey hint text. This is now consistent with the console prompt and sources panel text editor. BUG=none ==========
1. let's establish a consistent vocabulary throughout the TextPrompt, as we discussed offline 2. can we have a test readable? https://codereview.chromium.org/2439223002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js (right): https://codereview.chromium.org/2439223002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:275: delete this._selectionTimeout; what does this do? https://codereview.chromium.org/2439223002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:348: if (this._prefixRange && hasCommonPrefix) { let's check that cursor is in the end of the prompt -- explicitly https://codereview.chromium.org/2439223002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:349: this._prefixRange.endColumn += text.length - this._previousText.length; this._prefixRange.endColumn = text.length; https://codereview.chromium.org/2439223002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:385: this._prefixRange = null; this should probably go inside clearAutocomplete - prefixRange doesn't relate to autocomplete element https://codereview.chromium.org/2439223002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:391: _setAutocompleteText: function(text) text -> suggestion https://codereview.chromium.org/2439223002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:543: this._prefixRange = new WebInspector.TextRange(0, beforeRange.toString().length, 0, beforeRange.toString().length + fullWordRange.toString().length); let's convert range to string only once - i remember this to be expensive https://codereview.chromium.org/2439223002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:666: var selection = this._element.getComponentSelection(); let's rely on the _prefixRange her
lushnikov@chromium.org changed reviewers: - lushnikov@chromium.org
einbinder@chromium.org changed reviewers: + lushnikov@chromium.org
ptal
lgtm https://codereview.chromium.org/2439223002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js (right): https://codereview.chromium.org/2439223002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:553: this._ghostTextElement.remove(); let's get rid of that
The CQ bit was checked by einbinder@chromium.org
https://codereview.chromium.org/2439223002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js (right): https://codereview.chromium.org/2439223002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/TextPrompt.js:553: this._ghostTextElement.remove(); On 2016/10/27 at 00:35:50, lushnikov wrote: > let's get rid of that Done.
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2439223002/#ps120001 (title: "Was removing ghostText twice")
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.
Description was changed from ========== DevTools: Use grey hint text for applied suggestion in TextPrompt Auto completions in TextPrompt used to appear as selected text. Now they always show up as grey hint text. This is now consistent with the console prompt and sources panel text editor. BUG=none ========== to ========== DevTools: Use grey hint text for applied suggestion in TextPrompt Auto completions in TextPrompt used to appear as selected text. Now they always show up as grey hint text. This is now consistent with the console prompt and sources panel text editor. BUG=none ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Use grey hint text for applied suggestion in TextPrompt Auto completions in TextPrompt used to appear as selected text. Now they always show up as grey hint text. This is now consistent with the console prompt and sources panel text editor. BUG=none ========== to ========== DevTools: Use grey hint text for applied suggestion in TextPrompt Auto completions in TextPrompt used to appear as selected text. Now they always show up as grey hint text. This is now consistent with the console prompt and sources panel text editor. BUG=none Committed: https://crrev.com/5ba3f4712604e0201423db169c25178db98a16a0 Cr-Commit-Position: refs/heads/master@{#427916} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/5ba3f4712604e0201423db169c25178db98a16a0 Cr-Commit-Position: refs/heads/master@{#427916} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
